[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has removed a vote on this change. Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Removed Verified-1 by Impala Public Jenkins -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 11 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. IMPALA-11284: Do non-optional rewrites for || and Between predicate IMPALA-6590 disabled expression rewrites for ValuesStmt. However, CompoundVerticalBarExpr (||) cannot be executed directly without rewrite. This is because it could either be an OR operation with boolean arguments or CONCAT function call with string arguments. Backend cannot evaluate a BetweenPredicate and relies on rewriting BetweenPredicate into a conjunctive or disjunctive CompoundPredicate. This patch enables non-optional expression rewrites for ValuesStmt with CompoundVerticalBarExpr or BetweenPredicate. Testing: - Extended ExprRewriterTest and Planner test to have values clause with || and Between predicate Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Reviewed-on: http://gerrit.cloudera.org:8080/18581 Reviewed-by: Michael Smith Reviewed-by: Daniel Becker Tested-by: Riza Suminto --- M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java M fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java M testdata/workloads/functional-query/queries/QueryTest/values.test 4 files changed, 54 insertions(+), 7 deletions(-) Approvals: Michael Smith: Looks good to me, but someone else must approve Daniel Becker: Looks good to me, approved Riza Suminto: Verified -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 12 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 11: Verified+1 > Patch Set 11: Verified-1 > > Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9711/ All tests pass except 1 known flaky test tracked at IMPALA-12266. -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 11 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Wed, 13 Sep 2023 20:54:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 11: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9711/ -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 11 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Wed, 13 Sep 2023 20:47:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 11: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 11 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Wed, 13 Sep 2023 16:09:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 11: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9711/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 11 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Wed, 13 Sep 2023 16:09:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 11: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 11 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Tue, 12 Sep 2023 18:44:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 11: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/13990/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 11 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Tue, 12 Sep 2023 17:23:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has uploaded a new patch set (#11) to the change originally created by Abhishek Rawat. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. IMPALA-11284: Do non-optional rewrites for || and Between predicate IMPALA-6590 disabled expression rewrites for ValuesStmt. However, CompoundVerticalBarExpr (||) cannot be executed directly without rewrite. This is because it could either be an OR operation with boolean arguments or CONCAT function call with string arguments. Backend cannot evaluate a BetweenPredicate and relies on rewriting BetweenPredicate into a conjunctive or disjunctive CompoundPredicate. This patch enables non-optional expression rewrites for ValuesStmt with CompoundVerticalBarExpr or BetweenPredicate. Testing: - Extended ExprRewriterTest and Planner test to have values clause with || and Between predicate Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c --- M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java M fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java M testdata/workloads/functional-query/queries/QueryTest/values.test 4 files changed, 54 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/18581/11 -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 11 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has posted comments on this change. (
http://gerrit.cloudera.org:8080/18581 )
Change subject: IMPALA-11284: Do non-optional rewrites for || and Between
predicate
..
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18581/10/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java
File fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java:
http://gerrit.cloudera.org:8080/#/c/18581/10/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java@91
PS10, Line 91: public void rewriteExprs(ExprRewriter rewriter) throws
AnalysisException {
> Since we've removed the function doc comment about leaving the function emp
Done
--
To view, visit http://gerrit.cloudera.org:8080/18581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c
Gerrit-Change-Number: 18581
Gerrit-PatchSet: 11
Gerrit-Owner: Abhishek Rawat
Gerrit-Reviewer: Abhishek Rawat
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kurt Deschler
Gerrit-Reviewer: Riza Suminto
Gerrit-Reviewer: Wenzhe Zhou
Gerrit-Comment-Date: Tue, 12 Sep 2023 16:56:50 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Daniel Becker has posted comments on this change. (
http://gerrit.cloudera.org:8080/18581 )
Change subject: IMPALA-11284: Do non-optional rewrites for || and Between
predicate
..
Patch Set 10: Code-Review+1
(1 comment)
Thanks Riza.
http://gerrit.cloudera.org:8080/#/c/18581/10/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java
File fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java:
http://gerrit.cloudera.org:8080/#/c/18581/10/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java@91
PS10, Line 91: public void rewriteExprs(ExprRewriter rewriter) throws
AnalysisException {
Since we've removed the function doc comment about leaving the function empty,
the below comment could be extended saying that only non-optional rewrites are
done.
--
To view, visit http://gerrit.cloudera.org:8080/18581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c
Gerrit-Change-Number: 18581
Gerrit-PatchSet: 10
Gerrit-Owner: Abhishek Rawat
Gerrit-Reviewer: Abhishek Rawat
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kurt Deschler
Gerrit-Reviewer: Riza Suminto
Gerrit-Reviewer: Wenzhe Zhou
Gerrit-Comment-Date: Tue, 12 Sep 2023 16:36:30 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 10: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/13987/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 10 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Tue, 12 Sep 2023 16:31:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java File fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java: http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java@92 PS9, Line 92: // IMPALA-11284: Don't skip rewrites for || and BETWEEN operator as the backend > I think that should be fine too. BetweenToCompoundRule and ExtractCompoundV Done -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 10 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Tue, 12 Sep 2023 16:06:00 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has uploaded a new patch set (#10) to the change originally created by Abhishek Rawat. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. IMPALA-11284: Do non-optional rewrites for || and Between predicate IMPALA-6590 disabled expression rewrites for ValuesStmt. However, CompoundVerticalBarExpr (||) cannot be executed directly without rewrite. This is because it could either be an OR operation with boolean arguments or CONCAT function call with string arguments. Backend cannot evaluate a BetweenPredicate and relies on rewriting BetweenPredicate into a conjunctive or disjunctive CompoundPredicate. This patch enables non-optional expression rewrites for ValuesStmt with CompoundVerticalBarExpr or BetweenPredicate. Testing: - Extended ExprRewriterTest and Planner test to have values clause with || and Between predicate Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c --- M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java M fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java M testdata/workloads/functional-query/queries/QueryTest/values.test 4 files changed, 51 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/18581/10 -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 10 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java File fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java: http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java@92 PS9, Line 92: if (!requireMandatoryRewrite_) return; > What if we didn't have the 'requireMandatoryRewrite_' field and instead jus I think that should be fine too. BetweenToCompoundRule and ExtractCompoundVerticalBarExprRule will return early if the predicate they're aiming does not exist in the operand. I will make this change in the next patch set. -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 9 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Tue, 12 Sep 2023 15:28:30 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Daniel Becker has posted comments on this change. (
http://gerrit.cloudera.org:8080/18581 )
Change subject: IMPALA-11284: Do non-optional rewrites for || and Between
predicate
..
Patch Set 9: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java
File fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java:
http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java@439
PS9, Line 439: private static boolean
isRequireMandatoryRewrite(List operands) {
> Looking again, I think requireMandatoryRewrite_ should not be reset like it
Thanks, it makes sense.
http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java
File fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java:
http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java@92
PS9, Line 92: if (!requireMandatoryRewrite_) return;
What if we didn't have the 'requireMandatoryRewrite_' field and instead just
had the below ExprRewriter in all cases? It would still only rewrite the
expressions that need to be rewritten.
It would make the code simpler, though maybe a bit less performant (the expr
tree would have to be traversed for both rules instead of checking once whether
any || or BETWEEN expr occurs in it). What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/18581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c
Gerrit-Change-Number: 18581
Gerrit-PatchSet: 9
Gerrit-Owner: Abhishek Rawat
Gerrit-Reviewer: Abhishek Rawat
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kurt Deschler
Gerrit-Reviewer: Riza Suminto
Gerrit-Reviewer: Wenzhe Zhou
Gerrit-Comment-Date: Tue, 12 Sep 2023 09:27:23 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has posted comments on this change. (
http://gerrit.cloudera.org:8080/18581 )
Change subject: IMPALA-11284: Do non-optional rewrites for || and Between
predicate
..
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java
File fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java:
http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java@439
PS9, Line 439: private static boolean
isRequireMandatoryRewrite(List operands) {
> In the version in PS7, 'equireMandatoryRewrite_' could be reset to false if
Looking again, I think requireMandatoryRewrite_ should not be reset like it did
in PS7.
Assignment in PS9 is the right one. It will enable mandatory rewrites for all
operands even if only one of them has CompoundVerticalBarExpr or
BetweenPredicate. The rewrites will become a no-op for operand that does not
require it.
--
To view, visit http://gerrit.cloudera.org:8080/18581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c
Gerrit-Change-Number: 18581
Gerrit-PatchSet: 9
Gerrit-Owner: Abhishek Rawat
Gerrit-Reviewer: Abhishek Rawat
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kurt Deschler
Gerrit-Reviewer: Riza Suminto
Gerrit-Reviewer: Wenzhe Zhou
Gerrit-Comment-Date: Mon, 11 Sep 2023 15:02:39 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Daniel Becker has posted comments on this change. (
http://gerrit.cloudera.org:8080/18581 )
Change subject: IMPALA-11284: Do non-optional rewrites for || and Between
predicate
..
Patch Set 9:
(1 comment)
Thanks Riza. I just have a question, I can +2 after that if nobody else wants
to review it.
http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java
File fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java:
http://gerrit.cloudera.org:8080/#/c/18581/9/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java@439
PS9, Line 439: private static boolean
isRequireMandatoryRewrite(List operands) {
In the version in PS7, 'equireMandatoryRewrite_' could be reset to false if the
'resultExpr' of some operand didn't contain || or BETWEEN. In this version it
can't be reset. Was it a bug in the older version?
--
To view, visit http://gerrit.cloudera.org:8080/18581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c
Gerrit-Change-Number: 18581
Gerrit-PatchSet: 9
Gerrit-Owner: Abhishek Rawat
Gerrit-Reviewer: Abhishek Rawat
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kurt Deschler
Gerrit-Reviewer: Riza Suminto
Gerrit-Reviewer: Wenzhe Zhou
Gerrit-Comment-Date: Mon, 11 Sep 2023 11:10:18 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 9: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 9 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Sat, 09 Sep 2023 00:45:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 9: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/13958/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 9 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Fri, 08 Sep 2023 20:45:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 9: (5 comments) http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java File fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java: http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java@186 PS7, Line 186: op > Nit: are. Done http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java@711 PS7, Line 711: // Add to aliasSMap so that column refs in "order by" can be resolved. > This function does things related to tuple materialization. it's not clear Moved this to analyzeOperands() http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java File fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java: http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java@95 PS7, Line 95: to t > Nit: to? Done http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java: http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@152 PS7, Line 152: // Values stmt - expression rewrites are not required in this test cases. > This is no longer completely true - we should update the comment to say som Done http://gerrit.cloudera.org:8080/#/c/18581/7/testdata/workloads/functional-query/queries/QueryTest/values.test File testdata/workloads/functional-query/queries/QueryTest/values.test: http://gerrit.cloudera.org:8080/#/c/18581/7/testdata/workloads/functional-query/queries/QueryTest/values.test@148 PS7, Line 148: ( > You don't need to create a table, you could run a select query like this: Done -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 9 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Fri, 08 Sep 2023 20:21:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has uploaded a new patch set (#9) to the change originally created by Abhishek Rawat. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. IMPALA-11284: Do non-optional rewrites for || and Between predicate IMPALA-6590 disabled expression rewrites for ValuesStmt. However, CompoundVerticalBarExpr (||) cannot be executed directly without rewrite. This is because it could either be an OR operation with boolean arguments or CONCAT function call with string arguments. Backend cannot evaluate a BetweenPredicate and relies on rewriting BetweenPredicate into a conjunctive or disjunctive CompoundPredicate. This patch enables non-optional expression rewrites for ValuesStmt with CompoundVerticalBarExpr or BetweenPredicate. Testing: - Extended ExprRewriterTest and Planner test to have values clause with || and Between predicate Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c --- M fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java M fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java M testdata/workloads/functional-query/queries/QueryTest/values.test 5 files changed, 77 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/18581/9 -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 9 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 8: Patch set 8 is a rebase. I will address the comments in the next patch set. -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 8 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Fri, 08 Sep 2023 18:46:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java File fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java: http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java@711 PS7, Line 711: requireMandatoryRewrite_ = resultExpr.contains( > Not sure I understand the question. This function does things related to tuple materialization. it's not clear when the new flag gets used. I would think the transform should be completed before any materialization logic. -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 7 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Fri, 08 Sep 2023 15:04:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Daniel Becker has posted comments on this change. (
http://gerrit.cloudera.org:8080/18581 )
Change subject: IMPALA-11284: Do non-optional rewrites for || and Between
predicate
..
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java
File fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java:
http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java@186
PS7, Line 186: is
Nit: are.
http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java
File fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java:
http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java@95
PS7, Line 95: into
Nit: to?
http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java:
http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@152
PS7, Line 152: // Values stmt - expression rewrites are disabled.
This is no longer completely true - we should update the comment to say
something like expression rewrites are enabled except for || and the Between
predicate, or that most expression rewrites are disabled.
http://gerrit.cloudera.org:8080/#/c/18581/7/testdata/workloads/functional-query/queries/QueryTest/values.test
File testdata/workloads/functional-query/queries/QueryTest/values.test:
http://gerrit.cloudera.org:8080/#/c/18581/7/testdata/workloads/functional-query/queries/QueryTest/values.test@148
PS7, Line 148: create table values_11284(col string, value boolean);
You don't need to create a table, you could run a select query like this:
select * from
(
values (concat("a", "b" || "c"), 1 <= 2 AND ((0.5 BETWEEN 0 AND 1) AND (true
|| false))),
("hello" || "world", 0 <= 1 || 0.5 < 0.6),
("impala", 4.0 BETWEEN 3.2 AND 4.1),
("sql", 'a' NOT BETWEEN 'b' AND 'c')
) t;
--
To view, visit http://gerrit.cloudera.org:8080/18581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c
Gerrit-Change-Number: 18581
Gerrit-PatchSet: 7
Gerrit-Owner: Abhishek Rawat
Gerrit-Reviewer: Abhishek Rawat
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kurt Deschler
Gerrit-Reviewer: Riza Suminto
Gerrit-Reviewer: Wenzhe Zhou
Gerrit-Comment-Date: Fri, 08 Sep 2023 13:29:46 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java File fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java: http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java@711 PS7, Line 711: requireMandatoryRewrite_ = resultExpr.contains( > Why is this here with the value transfer logic instead of in Analyze? Not sure I understand the question. This is done at SetOperationStmt.createMetadata() method, which is called by SetOperationStmt.analyze() method. -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 7 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 07 Sep 2023 23:34:05 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java File fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java: http://gerrit.cloudera.org:8080/#/c/18581/7/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java@711 PS7, Line 711: requireMandatoryRewrite_ = resultExpr.contains( Why is this here with the value transfer logic instead of in Analyze? -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 7 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 07 Sep 2023 23:18:33 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 7: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 7 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 07 Sep 2023 21:38:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/13605/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 7 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Fri, 21 Jul 2023 00:45:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/18581/6/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java File fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java: http://gerrit.cloudera.org:8080/#/c/18581/6/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java@92 PS6, Line 92: (!requireMandatoryRewrite_) return; > This can probably be simplified more by creating new ExprRewriter that only Done -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 7 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Fri, 21 Jul 2023 00:24:40 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has uploaded a new patch set (#7) to the change originally created by Abhishek Rawat. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. IMPALA-11284: Do non-optional rewrites for || and Between predicate IMPALA-6590 disabled expression rewrites for ValuesStmt. However, CompoundVerticalBarExpr (||) cannot be executed directly without rewrite. This is because it could either be an OR operation with boolean arguments or CONCAT function call with string arguments. Backend cannot evaluate a BetweenPredicate and relies on rewriting BetweenPredicate into a conjunctive or disjunctive CompoundPredicate. This patch enables non-optional expression rewrites for ValuesStmt with CompoundVerticalBarExpr or BetweenPredicate. Testing: - Extended ExprRewriterTest and Planner test to have values clause with || and Between predicate Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c --- M fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java M fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java M testdata/workloads/functional-query/queries/QueryTest/values.test 5 files changed, 62 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/18581/7 -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 7 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/18581/6/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java File fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java: http://gerrit.cloudera.org:8080/#/c/18581/6/fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java@92 PS6, Line 92: ExprRewriter nonOptionalRewriter = rewriter.copyNonOptionalRewriter(); This can probably be simplified more by creating new ExprRewriter that only consist of BetweenToCompoundRule and ExtractCompoundVerticalBarExprRule. -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 6 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 20 Jul 2023 22:53:13 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/13599/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 6 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 20 Jul 2023 19:37:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/13598/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 5 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 20 Jul 2023 19:30:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has uploaded a new patch set (#6) to the change originally created by Abhishek Rawat. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. IMPALA-11284: Do non-optional rewrites for || and Between predicate IMPALA-6590 disabled expression rewrites for ValuesStmt. However, CompoundVerticalBarExpr (||) cannot be executed directly without rewrite. This is because it could either be an OR operation with boolean arguments or CONCAT function call with string arguments. Backend cannot evaluate a BetweenPredicate and relies on rewriting BetweenPredicate into a conjunctive or disjunctive CompoundPredicate. This patch enables non-optional expression rewrites for ValuesStmt with CompoundVerticalBarExpr or BetweenPredicate. Testing: - Extended ExprRewriterTest and Planner test to have values clause with || and Between predicate Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java M fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java M testdata/workloads/functional-query/queries/QueryTest/values.test 6 files changed, 97 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/18581/6 -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 6 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has posted comments on this change. (
http://gerrit.cloudera.org:8080/18581 )
Change subject: IMPALA-11284: Do non-optional rewrites for || and Between
predicate
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18581/3/testdata/workloads/functional-query/queries/QueryTest/values.test
File testdata/workloads/functional-query/queries/QueryTest/values.test:
http://gerrit.cloudera.org:8080/#/c/18581/3/testdata/workloads/functional-query/queries/QueryTest/values.test@149
PS3, Line 149: concat("a", "b" || "c"), 1 <= 2 AND ((0
> Looked into this a bit and realized that Impala already distinguishes betwe
Done
--
To view, visit http://gerrit.cloudera.org:8080/18581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c
Gerrit-Change-Number: 18581
Gerrit-PatchSet: 5
Gerrit-Owner: Abhishek Rawat
Gerrit-Reviewer: Abhishek Rawat
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Reviewer: Wenzhe Zhou
Gerrit-Comment-Date: Thu, 20 Jul 2023 19:09:07 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Riza Suminto has uploaded a new patch set (#5) to the change originally created by Abhishek Rawat. ( http://gerrit.cloudera.org:8080/18581 ) Change subject: IMPALA-11284: Do non-optional rewrites for || and Between predicate .. IMPALA-11284: Do non-optional rewrites for || and Between predicate IMPALA-6590 disabled expression rewrites for ValuesStmt. However, CompoundVerticalBarExpr (||) cannot be executed directly without rewrite. This is because it could either be an OR operation with boolean arguments or CONCAT function call with string arguments. Backend cannot evaluate a BetweenPredicate and relies on rewriting BetweenPredicate into a conjunctive or disjunctive CompoundPredicate. This patch enables non-optional expression rewrites for ValuesStmt with CompoundVerticalBarExpr or BetweenPredicate. Testing: - Extended ExprRewriterTest and Planner test to have values clause with || and Between predicate Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java M fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java M testdata/workloads/functional-query/queries/QueryTest/values.test 6 files changed, 94 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/18581/5 -- To view, visit http://gerrit.cloudera.org:8080/18581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c Gerrit-Change-Number: 18581 Gerrit-PatchSet: 5 Gerrit-Owner: Abhishek Rawat Gerrit-Reviewer: Abhishek Rawat Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-11284: Do non-optional rewrites for || and Between predicate
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/18581 )
Change subject: IMPALA-11284: Do non-optional rewrites for || and Between
predicate
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18581/5/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java:
http://gerrit.cloudera.org:8080/#/c/18581/5/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@164
PS5, Line 164: "values(1 <= 2 AND ((0.5 BETWEEN 0 AND 1) AND (('a' ||
'b') = 'ab' AND (true || false",
line too long (99 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/18581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99b8b33bf6468d12b9e26f0a6e744feb7072619c
Gerrit-Change-Number: 18581
Gerrit-PatchSet: 5
Gerrit-Owner: Abhishek Rawat
Gerrit-Reviewer: Abhishek Rawat
Gerrit-Reviewer: Aman Sinha
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Reviewer: Wenzhe Zhou
Gerrit-Comment-Date: Thu, 20 Jul 2023 19:08:27 +
Gerrit-HasComments: Yes
