Jackie-Jiang commented on a change in pull request #7590:
URL: https://github.com/apache/pinot/pull/7590#discussion_r736037212
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -39,6 +39,7 @@
import java.util.concurrent.atomic.AtomicLong;
import javax.annotation.Nullable;
import javax.annotation.concurrent.ThreadSafe;
+import javax.validation.constraints.NotNull;
Review comment:
(minor) We usually only annotate `Nullable` and treat arguments as
`NotNull` by default. Having both `Nullable` and `NotNull` can be slightly hard
to read
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -1413,16 +1457,19 @@ private void fixColumnName(String rawTableName,
Expression expression, @Nullable
* - Case-insensitive cluster
* - Column name in the format of [table_name].[column_name]
*/
- private String getActualColumnName(String rawTableName, String columnName,
- @Nullable Map<String, String> columnNameMap) {
+ private static String getActualColumnName(String rawTableName, String
columnName,
+ @Nullable Map<String, String> columnNameMap, @Nullable Map<String,
String> aliasMap, boolean isCaseInsensitive) {
// Check if column is in the format of [table_name].[column_name]
String[] splits = StringUtils.split(columnName, ".", 2);
- if (_tableCache.isCaseInsensitive()) {
+ if (isCaseInsensitive) {
if (splits.length == 2 && rawTableName.equalsIgnoreCase(splits[0])) {
columnName = splits[1];
}
- if (columnNameMap != null) {
- return columnNameMap.getOrDefault(columnName.toLowerCase(),
columnName);
+ String columnNameLowerCase = columnName.toLowerCase();
+ if (columnNameMap != null &&
columnNameMap.containsKey(columnNameLowerCase)) {
+ return columnNameMap.get(columnNameLowerCase);
+ } else if (aliasMap != null) {
+ return aliasMap.getOrDefault(columnNameLowerCase, columnName);
Review comment:
The column existence can be checked within this method:
```suggestion
if (columName.equals("*") {
return columnName;
}
String columnNameLowerCase = columnName.toLowerCase();
if (columnNameMap != null) {
String actualColumnName = columnNameMap.get(columnNameLowerCase);
if (actualColumnName != null) {
return actualColumnName;
}
}
if (aliasMap != null) {
String actualAlias = aliasMap.get(columnNameLowerCase);
if (actualAlias != null) {
return actualAlias;
}
}
throw new BadQueryRequestException(...);
```
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -1413,16 +1457,19 @@ private void fixColumnName(String rawTableName,
Expression expression, @Nullable
* - Case-insensitive cluster
* - Column name in the format of [table_name].[column_name]
*/
- private String getActualColumnName(String rawTableName, String columnName,
- @Nullable Map<String, String> columnNameMap) {
+ private static String getActualColumnName(String rawTableName, String
columnName,
+ @Nullable Map<String, String> columnNameMap, @Nullable Map<String,
String> aliasMap, boolean isCaseInsensitive) {
// Check if column is in the format of [table_name].[column_name]
String[] splits = StringUtils.split(columnName, ".", 2);
- if (_tableCache.isCaseInsensitive()) {
+ if (isCaseInsensitive) {
if (splits.length == 2 && rawTableName.equalsIgnoreCase(splits[0])) {
columnName = splits[1];
}
- if (columnNameMap != null) {
- return columnNameMap.getOrDefault(columnName.toLowerCase(),
columnName);
+ String columnNameLowerCase = columnName.toLowerCase();
+ if (columnNameMap != null &&
columnNameMap.containsKey(columnNameLowerCase)) {
+ return columnNameMap.get(columnNameLowerCase);
+ } else if (aliasMap != null) {
+ return aliasMap.getOrDefault(columnNameLowerCase, columnName);
} else {
return columnName;
}
Review comment:
We also want to check column existence for case sensitive case. The
existence check code can be moved out of the `if (isCaseInsensitive)` and can
be shared
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -1396,14 +1413,41 @@ private void fixColumnName(String rawTableName,
TransformExpressionTree expressi
/**
* Fixes the column names to the actual column names in the given SQL
expression.
*/
- private void fixColumnName(String rawTableName, Expression expression,
@Nullable Map<String, String> columnNameMap) {
+ @VisibleForTesting
+ private static void fixColumnName(String rawTableName, Expression expression,
+ @NotNull Map<String, String> columnNameMap, @NotNull Map<String, String>
aliasMap, boolean isCaseInsensitive) {
ExpressionType expressionType = expression.getType();
if (expressionType == ExpressionType.IDENTIFIER) {
Identifier identifier = expression.getIdentifier();
- identifier.setName(getActualColumnName(rawTableName,
identifier.getName(), columnNameMap));
+ final String actualColumnName =
+ getActualColumnName(rawTableName, identifier.getName(),
columnNameMap, aliasMap, isCaseInsensitive);
+ if (columnNameMap.containsValue(actualColumnName)) {
Review comment:
`Map.containsValue()` can be quite expensive. Instead, we can avoid it
by checking the column existence within the `getActualColumnName()`
--
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
