You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/07/23 00:57:46 UTC
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5734: Adding column name rewrite for the identifiers in the format of [table].[column]
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org