Jackie-Jiang commented on a change in pull request #5734:
URL: https://github.com/apache/incubator-pinot/pull/5734#discussion_r459160171
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -202,12 +202,10 @@ public BrokerResponse handleRequest(JsonNode request,
@Nullable RequesterIdentit
}
}
updateQuerySource(brokerRequest);
- if (_enableCaseInsensitive) {
- try {
- handleCaseSensitivity(brokerRequest);
- } catch (Exception e) {
- LOGGER.warn("Caught exception while rewriting PQL to make it
case-insensitive {}: {}, {}", requestId, query, e);
- }
+ try {
+ handleUpdateColumnNames(brokerRequest);
Review comment:
```suggestion
updateColumnNames(brokerRequest);
```
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -202,12 +202,10 @@ public BrokerResponse handleRequest(JsonNode request,
@Nullable RequesterIdentit
}
}
updateQuerySource(brokerRequest);
- if (_enableCaseInsensitive) {
- try {
- handleCaseSensitivity(brokerRequest);
- } catch (Exception e) {
- LOGGER.warn("Caught exception while rewriting PQL to make it
case-insensitive {}: {}, {}", requestId, query, e);
- }
+ try {
+ handleUpdateColumnNames(brokerRequest);
+ } catch (Exception e) {
+ LOGGER.warn("Caught exception while rewriting Column names in Pinot
Query {}: {}, {}", requestId, query, e);
Review comment:
```suggestion
LOGGER.warn("Caught exception while updating column names for query
{}: {}, {}", requestId, query, e);
```
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -704,74 +705,96 @@ private void handleCaseSensitivity(BrokerRequest
brokerRequest) {
for (int i = 0; i < selectionColumns.size(); i++) {
String expression = selectionColumns.get(i);
if (!expression.equals("*")) {
- selectionColumns.set(i, fixColumnNameCase(actualTableName,
expression));
+ selectionColumns.set(i, fixColumnName(tableName, expression));
}
}
}
if (brokerRequest.isSetOrderBy()) {
List<SelectionSort> orderBy = brokerRequest.getOrderBy();
for (SelectionSort selectionSort : orderBy) {
String expression = selectionSort.getColumn();
- selectionSort.setColumn(fixColumnNameCase(actualTableName,
expression));
+ selectionSort.setColumn(fixColumnName(tableName, expression));
}
}
PinotQuery pinotQuery = brokerRequest.getPinotQuery();
if (pinotQuery != null) {
- pinotQuery.getDataSource().setTableName(actualTableName);
+ pinotQuery.getDataSource().setTableName(tableName);
for (Expression expression : pinotQuery.getSelectList()) {
- fixColumnNameCase(actualTableName, expression);
+ fixColumnName(tableName, expression);
}
Expression filterExpression = pinotQuery.getFilterExpression();
if (filterExpression != null) {
- fixColumnNameCase(actualTableName, filterExpression);
+ fixColumnName(tableName, filterExpression);
}
List<Expression> groupByList = pinotQuery.getGroupByList();
if (groupByList != null) {
for (Expression expression : groupByList) {
- fixColumnNameCase(actualTableName, expression);
+ fixColumnName(tableName, expression);
}
}
List<Expression> orderByList = pinotQuery.getOrderByList();
if (orderByList != null) {
for (Expression expression : orderByList) {
- fixColumnNameCase(actualTableName, expression);
+ fixColumnName(tableName, expression);
}
}
Expression havingExpression = pinotQuery.getHavingExpression();
if (havingExpression != null) {
- fixColumnNameCase(actualTableName, havingExpression);
+ fixColumnName(tableName, havingExpression);
}
}
}
- private String fixColumnNameCase(String tableNameWithType, String
expression) {
+ private String fixColumnName(String tableNameWithType, String expression) {
TransformExpressionTree expressionTree =
TransformExpressionTree.compileToExpressionTree(expression);
- fixColumnNameCase(tableNameWithType, expressionTree);
+ fixColumnName(tableNameWithType, expressionTree);
return expressionTree.toString();
}
- private void fixColumnNameCase(String tableNameWithType,
TransformExpressionTree expression) {
+ private void fixColumnName(String tableNameWithType, TransformExpressionTree
expression) {
TransformExpressionTree.ExpressionType expressionType =
expression.getExpressionType();
if (expressionType == TransformExpressionTree.ExpressionType.IDENTIFIER) {
- expression.setValue(_tableCache.getActualColumnName(tableNameWithType,
expression.getValue()));
+ String identifier = expression.getValue();
+ expression.setValue(getActualColumnName(tableNameWithType, identifier));
} else if (expressionType ==
TransformExpressionTree.ExpressionType.FUNCTION) {
for (TransformExpressionTree child : expression.getChildren()) {
- fixColumnNameCase(tableNameWithType, child);
+ fixColumnName(tableNameWithType, child);
}
}
}
- private void fixColumnNameCase(String tableNameWithType, Expression
expression) {
+ private void fixColumnName(String tableNameWithType, Expression expression) {
ExpressionType expressionType = expression.getType();
if (expressionType == ExpressionType.IDENTIFIER) {
Identifier identifier = expression.getIdentifier();
- identifier.setName(_tableCache.getActualColumnName(tableNameWithType,
identifier.getName()));
+ identifier.setName(getActualColumnName(tableNameWithType,
identifier.getName()));
} else if (expressionType == ExpressionType.FUNCTION) {
for (Expression operand : expression.getFunctionCall().getOperands()) {
- fixColumnNameCase(tableNameWithType, operand);
+ fixColumnName(tableNameWithType, operand);
+ }
+ }
+ }
+
+ private String getActualColumnName(String tableNameWithType, String
columnName) {
+ String[] splits = StringUtils.split(columnName, ".", 2);
+ if (_enableCaseInsensitive) {
+ if (splits.length == 2) {
+ if (tableNameWithType.equalsIgnoreCase(splits[0]) ||
TableNameBuilder.extractRawTableName(tableNameWithType)
Review comment:
Not sure if we need the first check. `SELECT myTable_OFFLINE.colA FROM
...` seems impossible from connector as `_OFFLINE` is Pinot internal concept
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -667,19 +665,22 @@ private void computeResultsForLiteral(Literal literal,
List<String> columnNames,
}
/**
- * Fixes the case-insensitive column names to the actual column names in the
given broker request.
+ * Fixes the column names to the actual column names in the given broker
request.
*/
- private void handleCaseSensitivity(BrokerRequest brokerRequest) {
- String inputTableName = brokerRequest.getQuerySource().getTableName();
- String actualTableName = _tableCache.getActualTableName(inputTableName);
- brokerRequest.getQuerySource().setTableName(actualTableName);
+ private void handleUpdateColumnNames(BrokerRequest brokerRequest) {
+ if (_enableCaseInsensitive) {
+ String inputTableName = brokerRequest.getQuerySource().getTableName();
+ String actualTableName = _tableCache.getActualTableName(inputTableName);
+ brokerRequest.getQuerySource().setTableName(actualTableName);
+ }
Review comment:
Should we move this part into the `updateQuerySource()`?
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -1179,6 +1179,74 @@ public void testCaseInsensitivity() {
}, 10_000L, "Failed to get results for case-insensitive queries");
}
+ @Test
+ public void testColumnNameContainsTableName() {
+ int daysSinceEpoch = 16138;
+ long secondsSinceEpoch = 16138 * 24 * 60 * 60;
+ List<String> baseQueries = Arrays.asList("SELECT * FROM mytable",
+ "SELECT DaysSinceEpoch, timeConvert(DaysSinceEpoch,'DAYS','SECONDS')
FROM mytable",
+ "SELECT DaysSinceEpoch, timeConvert(DaysSinceEpoch,'DAYS','SECONDS')
FROM mytable order by DaysSinceEpoch limit 10000",
+ "SELECT DaysSinceEpoch, timeConvert(DaysSinceEpoch,'DAYS','SECONDS')
FROM mytable order by timeConvert(DaysSinceEpoch,'DAYS','SECONDS') DESC limit
10000",
+ "SELECT count(*) FROM mytable WHERE DaysSinceEpoch = " +
daysSinceEpoch,
+ "SELECT count(*) FROM mytable WHERE
timeConvert(DaysSinceEpoch,'DAYS','SECONDS') = " + secondsSinceEpoch,
+ "SELECT count(*) FROM mytable WHERE
timeConvert(DaysSinceEpoch,'DAYS','SECONDS') = " + daysSinceEpoch,
+ "SELECT MAX(timeConvert(DaysSinceEpoch,'DAYS','SECONDS')) FROM
mytable",
+ "SELECT COUNT(*) FROM mytable GROUP BY
dateTimeConvert(DaysSinceEpoch,'1:DAYS:EPOCH','1:HOURS:EPOCH','1:HOURS')");
+ List<String> queries = new ArrayList<>();
+ baseQueries.stream().forEach(q -> queries.add(q.replace("DaysSinceEpoch",
"mytable.DAYSSinceEpOch")));
Review comment:
```suggestion
baseQueries.forEach(q -> queries.add(q.replace("DaysSinceEpoch",
"mytable.DAYSSinceEpOch")));
```
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -1179,6 +1179,74 @@ public void testCaseInsensitivity() {
}, 10_000L, "Failed to get results for case-insensitive queries");
}
+ @Test
+ public void testColumnNameContainsTableName() {
+ int daysSinceEpoch = 16138;
+ long secondsSinceEpoch = 16138 * 24 * 60 * 60;
+ List<String> baseQueries = Arrays.asList("SELECT * FROM mytable",
+ "SELECT DaysSinceEpoch, timeConvert(DaysSinceEpoch,'DAYS','SECONDS')
FROM mytable",
+ "SELECT DaysSinceEpoch, timeConvert(DaysSinceEpoch,'DAYS','SECONDS')
FROM mytable order by DaysSinceEpoch limit 10000",
+ "SELECT DaysSinceEpoch, timeConvert(DaysSinceEpoch,'DAYS','SECONDS')
FROM mytable order by timeConvert(DaysSinceEpoch,'DAYS','SECONDS') DESC limit
10000",
+ "SELECT count(*) FROM mytable WHERE DaysSinceEpoch = " +
daysSinceEpoch,
+ "SELECT count(*) FROM mytable WHERE
timeConvert(DaysSinceEpoch,'DAYS','SECONDS') = " + secondsSinceEpoch,
+ "SELECT count(*) FROM mytable WHERE
timeConvert(DaysSinceEpoch,'DAYS','SECONDS') = " + daysSinceEpoch,
+ "SELECT MAX(timeConvert(DaysSinceEpoch,'DAYS','SECONDS')) FROM
mytable",
+ "SELECT COUNT(*) FROM mytable GROUP BY
dateTimeConvert(DaysSinceEpoch,'1:DAYS:EPOCH','1:HOURS:EPOCH','1:HOURS')");
+ List<String> queries = new ArrayList<>();
+ baseQueries.stream().forEach(q -> queries.add(q.replace("DaysSinceEpoch",
"mytable.DAYSSinceEpOch")));
+ baseQueries.stream().forEach(q -> queries.add(q.replace("DaysSinceEpoch",
"mytable.DAYSSinceEpOch")));
+
+ // Wait for at most 10 seconds for broker to get the ZK callback of the
schema change
+ TestUtils.waitForCondition(aVoid -> {
Review comment:
Remove the `waitForCondition` as there is no schema change. Same for
`testCaseInsensitivity()` and
`testCaseInsensitivityWithColumnNameContainsTableName()`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
