You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2023/09/18 09:07:06 UTC
[GitHub] [pinot] Jackie-Jiang opened a new pull request, #11610: Fix the alias handling in single-stage engine
Jackie-Jiang opened a new pull request, #11610:
URL: https://github.com/apache/pinot/pull/11610
Fix 2 issues:
- Fix #11597
- Alias should not be applied to filter (SQL standard)
--
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-unsubscribe@pinot.apache.org
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
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #11610: Fix the alias handling in single-stage engine
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
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
[GitHub] [pinot] xiangfu0 merged pull request #11610: Fix the alias handling in single-stage engine
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #11610:
URL: https://github.com/apache/pinot/pull/11610
--
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-unsubscribe@pinot.apache.org
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
[GitHub] [pinot] codecov-commenter commented on pull request #11610: Fix the alias handling in single-stage engine
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11610:
URL: https://github.com/apache/pinot/pull/11610#issuecomment-1723079012
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11610?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11610](https://app.codecov.io/gh/apache/pinot/pull/11610?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (efa1e8a) into [master](https://app.codecov.io/gh/apache/pinot/commit/a8411c02d92b10af462592f49e7b10c0c2cb8f76?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (a8411c0) will **decrease** coverage by `48.70%`.
> Report is 1 commits behind head on master.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11610 +/- ##
=============================================
- Coverage 63.20% 14.51% -48.70%
+ Complexity 1107 201 -906
=============================================
Files 2323 2323
Lines 124465 124452 -13
Branches 18989 18986 -3
=============================================
- Hits 78672 18062 -60610
- Misses 40204 104871 +64667
+ Partials 5589 1519 -4070
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11610/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/11610/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/11610/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/11610/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/11610/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-17](https://app.codecov.io/gh/apache/pinot/pull/11610/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.51% <0.00%> (-48.56%)` | :arrow_down: |
| [java-20](https://app.codecov.io/gh/apache/pinot/pull/11610/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11610/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.51% <0.00%> (-48.70%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11610/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.51% <0.00%> (-48.70%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11610/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/11610/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.51% <0.00%> (-0.04%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files Changed](https://app.codecov.io/gh/apache/pinot/pull/11610?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...pache/pinot/sql/parsers/rewriter/AliasApplier.java](https://app.codecov.io/gh/apache/pinot/pull/11610?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9BbGlhc0FwcGxpZXIuamF2YQ==) | `0.00% <0.00%> (-96.30%)` | :arrow_down: |
| [...not/sql/parsers/rewriter/QueryRewriterFactory.java](https://app.codecov.io/gh/apache/pinot/pull/11610?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9RdWVyeVJld3JpdGVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
... and [1503 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11610/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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-unsubscribe@pinot.apache.org
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
[GitHub] [pinot] xiangfu0 commented on pull request #11610: Fix the alias handling in single-stage engine
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #11610:
URL: https://github.com/apache/pinot/pull/11610#issuecomment-1724827392
> Just for curiosity, what regression do you observe when the #10815 is introduced?
master branch build failure:
https://github.com/apache/pinot/actions/runs/6230277100/job/16910046883
--
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-unsubscribe@pinot.apache.org
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
[GitHub] [pinot] wirybeaver commented on pull request #11610: Fix the alias handling in single-stage engine
Posted by "wirybeaver (via GitHub)" <gi...@apache.org>.
wirybeaver commented on PR #11610:
URL: https://github.com/apache/pinot/pull/11610#issuecomment-1724826090
Just for curiosity, what regression do you observe when the https://github.com/apache/pinot/pull/10815 is introduced?
--
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-unsubscribe@pinot.apache.org
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