yashmayya commented on code in PR #15994:
URL: https://github.com/apache/pinot/pull/15994#discussion_r2144492211
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/LogicalTableConfigUtils.java:
##########
@@ -139,13 +139,22 @@ public static void validateLogicalTableConfig(
"Invalid logical table. Reason: 'physicalTableConfigMap' should not
be null or empty");
}
+ String databaseName =
DatabaseUtils.extractDatabaseFromFullyQualifiedTableName(logicalTableConfig.getTableName());
Set<String> offlineTableNames = new HashSet<>();
Set<String> realtimeTableNames = new HashSet<>();
for (Map.Entry<String, PhysicalTableConfig> entry :
logicalTableConfig.getPhysicalTableConfigMap().entrySet()) {
String physicalTableName = entry.getKey();
PhysicalTableConfig physicalTableConfig = entry.getValue();
+ // validate database name matches
+ String physicalTableDatabaseName =
DatabaseUtils.extractDatabaseFromFullyQualifiedTableName(physicalTableName);
+ if (!StringUtils.equalsIgnoreCase(databaseName,
physicalTableDatabaseName)) {
+ throw new IllegalArgumentException(
+ "Invalid logical table. Reason: 'physicalTableName' should have
the same database name as logical table: "
Review Comment:
> Reason: 'physicalTableName' should have the same database name as logical
table
There doesn't seem to be any config key defined as `physicalTableName`?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotLogicalTableResource.java:
##########
@@ -141,10 +143,31 @@ public SuccessResponse addLogicalTable(
ResourceUtils.checkPermissionAndAccess(tableName, request, httpHeaders,
AccessType.CREATE,
Actions.Table.CREATE_TABLE, _accessControlFactory, LOGGER);
+ translatePhysicalTableNamesWithDB(logicalTableConfig, httpHeaders);
SuccessResponse successResponse = addLogicalTable(logicalTableConfig);
return new ConfigSuccessResponse(successResponse.getStatus(),
logicalTableConfigAndUnrecognizedProps.getRight());
}
+ private void translatePhysicalTableNamesWithDB(LogicalTableConfig
logicalTableConfig, HttpHeaders headers) {
+ // Translate physical table names to include the database name
+ Map<String, PhysicalTableConfig> physicalTableConfigMap = new HashMap<>();
+ for (Map.Entry<String, PhysicalTableConfig> entry :
logicalTableConfig.getPhysicalTableConfigMap().entrySet()) {
+ String physicalTableName =
DatabaseUtils.translateTableName(entry.getKey(), headers);
+ physicalTableConfigMap.put(physicalTableName, entry.getValue());
+ }
+ logicalTableConfig.setPhysicalTableConfigMap(physicalTableConfigMap);
Review Comment:
nit: could this be replaced with a functional approach? Something like:
```
Map<String, PhysicalTableConfig> physicalTableConfigMap =
logicalTableConfig.getPhysicalTableConfigMap().entrySet()
.stream()
.map(entry ->
Map.entry(DatabaseUtils.translateTableName(entry.getKey(), headers),
entry.getValue()))
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
