Jackie-Jiang commented on code in PR #8713:
URL: https://github.com/apache/pinot/pull/8713#discussion_r882201954
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -762,29 +763,34 @@ private void handleSubquery(Expression expression, long
requestId, JsonNode json
* - Case-insensitive cluster
* - Table name in the format of [database_name].[table_name]
*
- * Drop the database part if there is no existing table in the format of
[database_name].[table_name], but only
- * [table_name].
+ * If the PinotConfiguration does not allow dots in table name, will do the
below:
Review Comment:
Let's revert the comment update since we are following the old behavior
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -762,29 +763,34 @@ private void handleSubquery(Expression expression, long
requestId, JsonNode json
* - Case-insensitive cluster
* - Table name in the format of [database_name].[table_name]
*
- * Drop the database part if there is no existing table in the format of
[database_name].[table_name], but only
- * [table_name].
+ * If the PinotConfiguration does not allow dots in table name, will do the
below:
+ * Drop the prefix part for the format of [database_name].[table_name], keep
only [table_name].
+ * If the PinotConfiguration allows dots in table name, we will not do the
split
+ * @param tableName the table name in the query
+ * @param tableCache the table case-sensitive cache
+ * @param routingManager the routing mananger for testing whether a route
exists
+ * @return the table name in the format of [database_name].[table_name]
*/
- private String getActualTableName(String tableName) {
+ @VisibleForTesting
+ static String getActualTableName(String tableName, TableCache tableCache,
BrokerRoutingManager routingManager) {
// Use TableCache to handle case-insensitive table name
- if (_tableCache.isCaseInsensitive()) {
- String actualTableName = _tableCache.getActualTableName(tableName);
+ if (tableCache.isIgnoreCase()) {
Review Comment:
I just realize that we have enhanced `tableCache` in #8475 to store table
name regardless of the case sensitivity. We can actually significantly simplify
this method by always using `tableCache` to resolve the actual table name:
```suggestion
static String getActualTableName(String tableName, TableCache tableCache) {
String actualTableName = tableCache.getActualTableName(tableName);
if (actualTableName != null) {
return actualTableName;
}
// Check if table is in the format of [database_name].[table_name]
String[] tableNameSplits = StringUtils.split(tableName, ".", 2);
if (tableNameSplits.length == 2) {
actualTableName = tableCache.getActualTableName(tableNameSplits[1]);
if (actualTableName != null) {
return actualTableName;
}
}
return tableName;
}
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -183,7 +184,9 @@ public ConfigSuccessResponse addTable(
TableConfigUtils.validate(tableConfig, schema, typesToSkip,
_controllerConf.isDisableIngestionGroovy());
// TableConfigUtils.validateTableName(...) checks table name rules.
// So it won't effect already created tables.
- TableConfigUtils.validateTableName(tableConfig);
+ boolean allowDots =
_controllerConf.getProperty(CommonConstants.Helix.ALLOW_TABLE_NAME_WITH_DATABASE,
Review Comment:
(minor) Rename to `allowTableNameWithDatabase`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java:
##########
@@ -408,19 +409,20 @@ private TableConfigs validateConfig(TableConfigs
tableConfigs, @Nullable String
Preconditions.checkState(rawTableName.equals(schema.getSchemaName()),
"'tableName': %s must be equal to 'schemaName' from 'schema': %s",
rawTableName, schema.getSchemaName());
SchemaUtils.validate(schema);
-
+ boolean allowDots =
_controllerConf.getProperty(CommonConstants.Helix.ALLOW_TABLE_NAME_WITH_DATABASE,
Review Comment:
(minor) Rename to `allowTableNameWithDatabase`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -153,10 +153,19 @@ private static Set<ValidationType>
parseTypesToSkipString(@Nullable String types
* <li>Table name shouldn't contain dot or space in it</li>
* </ul>
*/
- public static void validateTableName(TableConfig tableConfig) {
+ public static void validateTableName(TableConfig tableConfig, boolean
allowDots) {
Review Comment:
(minor) Rename to `allowTableNameWithDatabase`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -153,10 +153,19 @@ private static Set<ValidationType>
parseTypesToSkipString(@Nullable String types
* <li>Table name shouldn't contain dot or space in it</li>
Review Comment:
Update the javadoc about the new validation logic
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -153,10 +153,19 @@ private static Set<ValidationType>
parseTypesToSkipString(@Nullable String types
* <li>Table name shouldn't contain dot or space in it</li>
* </ul>
*/
- public static void validateTableName(TableConfig tableConfig) {
+ public static void validateTableName(TableConfig tableConfig, boolean
allowDots) {
String tableName = tableConfig.getTableName();
- if (tableName.contains(".") || tableName.contains(" ")) {
- throw new IllegalStateException("Table name: '" + tableName + "'
containing '.' or space is not allowed");
+ int dotCount = StringUtils.countMatches(tableName, '.');
+ // For transitioning into full [database_name].[table_name] support, we
allow the table name
+ // with one dot at max, so the admin may create mydb.mytable with a
feature knob.
+ if (allowDots && dotCount > 1) {
+ throw new IllegalStateException("Table name: '" + tableName + "'
containing more than one '.' is not allowed");
+ }
+ if (!allowDots && dotCount > 0) {
+ throw new IllegalStateException("Table name: '" + tableName + "'
containing '.' is not allowed");
+ }
+ if (tableName.contains(" ")) {
Review Comment:
Not introduced in this PR, but more robust
```suggestion
if (StringUtils.containsWhitespace(tableName)) {
```
--
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
