[Impala-ASF-CR] IMPALA-4213: Planner not pushing some Kudu predicates
Alex Behm has posted comments on this change. Change subject: IMPALA-4213: Planner not pushing some Kudu predicates .. Patch Set 3: Code-Review+2 (3 comments) http://gerrit.cloudera.org:8080/#/c/4613/3//COMMIT_MSG Commit Message: Line 7: IMPALA-4213: Planner not pushing some Kudu predicates Commit msg should describe the fix, bot the problem (or JIRA title). Something like "Fix pushing predicates to Kudu that need constant folding" http://gerrit.cloudera.org:8080/#/c/4613/3/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java File fe/src/main/java/org/apache/impala/planner/KuduScanNode.java: Line 288: predicate = KuduScanNode.normalizeSlotRefComparison(predicate, analyzer); can remove KuduScanNode Line 290: ComparisonOp op = getKuduOperator(((BinaryPredicate)predicate).getOp()); can remove cast to BinaryPredicate -- To view, visit http://gerrit.cloudera.org:8080/4613 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3bab22d90ee00a054c847de6c734b4f24a3f5a85 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4213: Planner not pushing some Kudu predicates
Matthew Jacobs has uploaded a new patch set (#3). Change subject: IMPALA-4213: Planner not pushing some Kudu predicates .. IMPALA-4213: Planner not pushing some Kudu predicates Folding const exprs where there were implicit casts on the slot resulted in the predicate not being pushed to Kudu. Change-Id: I3bab22d90ee00a054c847de6c734b4f24a3f5a85 --- M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java M testdata/workloads/functional-planner/queries/PlannerTest/kudu.test 4 files changed, 77 insertions(+), 56 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/4613/3 -- To view, visit http://gerrit.cloudera.org:8080/4613 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3bab22d90ee00a054c847de6c734b4f24a3f5a85 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Matthew Jacobs
[Impala-ASF-CR] IMPALA-4213: Planner not pushing some Kudu predicates
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-4213: Planner not pushing some Kudu predicates
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4613/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java:
Line 124: try {
> Got it, I agree with you. We should fold first.
Done
http://gerrit.cloudera.org:8080/#/c/4613/2/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
Line 1195:* resulting LiteralExpr. Modifies 'this' in place and resets it.
Hence, it is not safe
> I don't think it "expects" an unanalyzed expr. I think it was just not nece
Done
--
To view, visit http://gerrit.cloudera.org:8080/4613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3bab22d90ee00a054c847de6c734b4f24a3f5a85
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Matthew Jacobs
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4213: Planner not pushing some Kudu predicates
Alex Behm has posted comments on this change.
Change subject: IMPALA-4213: Planner not pushing some Kudu predicates
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4613/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java:
Line 124: try {
> So now there are basically 2 things we have to do:
Got it, I agree with you. We should fold first.
Btw, while we are at it, let's move this function into the KuduScanNode
http://gerrit.cloudera.org:8080/#/c/4613/2/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
Line 1195:* resulting LiteralExpr. Modifies 'this' in place and resets it.
Hence, it is not safe
> Works for me, though there is another caller: HdfsPartitionPruner.canEvalUs
I don't think it "expects" an unanalyzed expr. I think it was just not
necessary for that use case so at the time we did not reset() and analyze.
However, to make the behavior of this function saner it seems we should do that.
--
To view, visit http://gerrit.cloudera.org:8080/4613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3bab22d90ee00a054c847de6c734b4f24a3f5a85
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Matthew Jacobs
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4213: Planner not pushing some Kudu predicates
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-4213: Planner not pushing some Kudu predicates
..
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4613/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java:
Line 124: try {
> Do we still need these changes? I kind of liked the previous flow better. I
So now there are basically 2 things we have to do:
a) fold the children (which now removes the unnecessary casts)
b) re-order the predicate so it matches: slot op literal
The advantage of doing (a) first is that we can directly check if one of the
children is a SlotRef, otherwise we have to assume there could be a cast in
there and use unwrapSlotRef(). That's not a big deal but it's just 1 less thing
to think about.
How would we be able to remove the analyze call which happens after the
foldConstantChildren()? That calls reset(). Or were you thinking that
foldConstantChildren() should also re-analyze? I'm not sure how that impacts
the other caller in HdfsPartitionPruner.canEvalUsingPartitionMd()
http://gerrit.cloudera.org:8080/#/c/4613/2/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
Line 1195:* resulting LiteralExpr. Modifies 'this' in place and resets it.
Hence, it is not safe
> Seems cleaner to reset() and then analyze() in here. That way callers don't
Works for me, though there is another caller:
HdfsPartitionPruner.canEvalUsingPartitionMd(). I'm not really sure what's going
on with that code, i.e. why it expects an unanalyzed expr and I don't see it
re-analyzing the expr after calling this. Does that look suspicious to you?
Line 1213: reset();
> Let's only do this if there was actually folding.
Done
--
To view, visit http://gerrit.cloudera.org:8080/4613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3bab22d90ee00a054c847de6c734b4f24a3f5a85
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Matthew Jacobs
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4213: Planner not pushing some Kudu predicates
Alex Behm has posted comments on this change.
Change subject: IMPALA-4213: Planner not pushing some Kudu predicates
..
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4613/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java:
Line 124: try {
Do we still need these changes? I kind of liked the previous flow better. I
think with the changes to foldConstantChildren() should be sufficient (we can
remove the analyzeNoThrow() from the original L145 though).
http://gerrit.cloudera.org:8080/#/c/4613/2/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
Line 1195:* resulting LiteralExpr. Modifies 'this' in place and resets it.
Hence, it is not safe
Seems cleaner to reset() and then analyze() in here. That way callers don't
need to worry about the weird state the modified Expr could be in. It just
works.
Line 1213: reset();
Let's only do this if there was actually folding.
--
To view, visit http://gerrit.cloudera.org:8080/4613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3bab22d90ee00a054c847de6c734b4f24a3f5a85
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Matthew Jacobs
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4213: Planner not pushing some Kudu predicates
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4213: Planner not pushing some Kudu predicates .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/4613/1/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java: Line 126: predicate.foldConstantChildren(analyzer); > I think foldConstantChildren() subtly does not do what we really want here. Done -- To view, visit http://gerrit.cloudera.org:8080/4613 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3bab22d90ee00a054c847de6c734b4f24a3f5a85 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4213: Planner not pushing some Kudu predicates
Matthew Jacobs has uploaded a new patch set (#2). Change subject: IMPALA-4213: Planner not pushing some Kudu predicates .. IMPALA-4213: Planner not pushing some Kudu predicates Folding const exprs where there were implicit casts on the slot resulted in the predicate not being pushed to Kudu. Change-Id: I3bab22d90ee00a054c847de6c734b4f24a3f5a85 --- M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/Expr.java M testdata/workloads/functional-planner/queries/PlannerTest/kudu.test 3 files changed, 32 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/4613/2 -- To view, visit http://gerrit.cloudera.org:8080/4613 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3bab22d90ee00a054c847de6c734b4f24a3f5a85 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Alex Behm
[Impala-ASF-CR] IMPALA-4213: Planner not pushing some Kudu predicates
Alex Behm has posted comments on this change. Change subject: IMPALA-4213: Planner not pushing some Kudu predicates .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/4613/1/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java: Line 126: predicate.foldConstantChildren(analyzer); I think foldConstantChildren() subtly does not do what we really want here. I'd suggest fixing foldConstantchildren() to reset() and analyze() the expr again. Note that the comment on foldConstantChildren() explicitly mentions with rather strange behavior. -- To view, visit http://gerrit.cloudera.org:8080/4613 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3bab22d90ee00a054c847de6c734b4f24a3f5a85 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Alex Behm Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4213: Planner not pushing some Kudu predicates
Matthew Jacobs has uploaded a new change for review. http://gerrit.cloudera.org:8080/4613 Change subject: IMPALA-4213: Planner not pushing some Kudu predicates .. IMPALA-4213: Planner not pushing some Kudu predicates Folding const exprs where there were implicit casts on the slot resulted in the predicate not being pushed to Kudu. Change-Id: I3bab22d90ee00a054c847de6c734b4f24a3f5a85 --- M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M testdata/workloads/functional-planner/queries/PlannerTest/kudu.test 2 files changed, 34 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/4613/1 -- To view, visit http://gerrit.cloudera.org:8080/4613 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I3bab22d90ee00a054c847de6c734b4f24a3f5a85 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs
