xiangfu0 commented on code in PR #11610:
URL: https://github.com/apache/pinot/pull/11610#discussion_r1329276237
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/AliasApplier.java:
##########
@@ -31,38 +29,30 @@
public class AliasApplier implements QueryRewriter {
+
@Override
public PinotQuery rewrite(PinotQuery pinotQuery) {
-
- // Update alias
- Map<Identifier, Expression> aliasMap =
extractAlias(pinotQuery.getSelectList());
+ Map<String, Expression> aliasMap =
extractAlias(pinotQuery.getSelectList());
applyAlias(aliasMap, pinotQuery);
-
- // Validate
- validateSelectionClause(aliasMap, pinotQuery);
return pinotQuery;
}
- private static Map<Identifier, Expression> extractAlias(List<Expression>
expressions) {
- Map<Identifier, Expression> aliasMap = new HashMap<>();
- for (Expression expression : expressions) {
- Function functionCall = expression.getFunctionCall();
- if (functionCall == null) {
+ private static Map<String, Expression> extractAlias(List<Expression>
selectExpressions) {
+ Map<String, Expression> aliasMap = new HashMap<>();
+ for (Expression expression : selectExpressions) {
+ Function function = expression.getFunctionCall();
+ if (function == null || !function.getOperator().equals("as")) {
continue;
}
- if (functionCall.getOperator().equals("as")) {
- Expression identifierExpr = functionCall.getOperands().get(1);
- aliasMap.put(identifierExpr.getIdentifier(),
functionCall.getOperands().get(0));
+ String alias = function.getOperands().get(1).getIdentifier().getName();
+ if (aliasMap.put(alias, function.getOperands().get(0)) != null) {
+ throw new SqlCompilationException("Find duplicate alias: " + alias);
}
}
return aliasMap;
}
- private static void applyAlias(Map<Identifier, Expression> aliasMap,
PinotQuery pinotQuery) {
- Expression filterExpression = pinotQuery.getFilterExpression();
Review Comment:
My concern here is that this will silently return empty results once users
upgrade.
Shall we instead throw exception if detected the alias in filter and
explicitly mention that alias shouldn't be used, so users can at least find out
the root cause and either roll back or fix queries before upgrade?
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/QueryRewriterFactory.java:
##########
@@ -33,10 +33,14 @@ private QueryRewriterFactory() {
private static final Logger LOGGER =
LoggerFactory.getLogger(QueryRewriterFactory.class);
+ // NOTE:
+ // OrdinalsUpdater must be applied after AliasApplier because
OrdinalsUpdater can put the select expression
+ // (reference) into the group-by list, but the alias should not be applied
to the reference.
+ // E.g. SELECT a + 1 AS a FROM table GROUP BY 1
public static final List<String> DEFAULT_QUERY_REWRITERS_CLASS_NAMES =
ImmutableList.of(CompileTimeFunctionsInvoker.class.getName(),
SelectionsRewriter.class.getName(),
- PredicateComparisonRewriter.class.getName(),
OrdinalsUpdater.class.getName(),
- AliasApplier.class.getName(),
NonAggregationGroupByToDistinctQueryRewriter.class.getName());
+ PredicateComparisonRewriter.class.getName(),
AliasApplier.class.getName(), OrdinalsUpdater.class.getName(),
Review Comment:
good catch!
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/AliasApplier.java:
##########
@@ -31,38 +29,30 @@
public class AliasApplier implements QueryRewriter {
+
@Override
public PinotQuery rewrite(PinotQuery pinotQuery) {
-
- // Update alias
- Map<Identifier, Expression> aliasMap =
extractAlias(pinotQuery.getSelectList());
+ Map<String, Expression> aliasMap =
extractAlias(pinotQuery.getSelectList());
applyAlias(aliasMap, pinotQuery);
-
- // Validate
- validateSelectionClause(aliasMap, pinotQuery);
return pinotQuery;
}
- private static Map<Identifier, Expression> extractAlias(List<Expression>
expressions) {
- Map<Identifier, Expression> aliasMap = new HashMap<>();
- for (Expression expression : expressions) {
- Function functionCall = expression.getFunctionCall();
- if (functionCall == null) {
+ private static Map<String, Expression> extractAlias(List<Expression>
selectExpressions) {
+ Map<String, Expression> aliasMap = new HashMap<>();
+ for (Expression expression : selectExpressions) {
+ Function function = expression.getFunctionCall();
+ if (function == null || !function.getOperator().equals("as")) {
continue;
}
- if (functionCall.getOperator().equals("as")) {
- Expression identifierExpr = functionCall.getOperands().get(1);
- aliasMap.put(identifierExpr.getIdentifier(),
functionCall.getOperands().get(0));
+ String alias = function.getOperands().get(1).getIdentifier().getName();
+ if (aliasMap.put(alias, function.getOperands().get(0)) != null) {
+ throw new SqlCompilationException("Find duplicate alias: " + alias);
Review Comment:
good catch!
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
