Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/7793 )
Change subject: IMPALA-4252: Min-max runtime filters for Kudu ...................................................................... Patch Set 11: (8 comments) http://gerrit.cloudera.org:8080/#/c/7793/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/7793/9//COMMIT_MSG@27 PS9, Line 27: > Thanks for explaining. https://docs.google.com/document/d/1G-SPZelateebNxzVb67urEVtjc5Itw-B_jjfS85bSCE/edit?usp=sharing http://gerrit.cloudera.org:8080/#/c/7793/11/be/src/exec/filter-context.h File be/src/exec/filter-context.h: http://gerrit.cloudera.org:8080/#/c/7793/11/be/src/exec/filter-context.h@118 PS11, Line 118: Materialize filter values. > what does that mean? Done http://gerrit.cloudera.org:8080/#/c/7793/11/be/src/runtime/runtime-filter-bank.h File be/src/runtime/runtime-filter-bank.h: http://gerrit.cloudera.org:8080/#/c/7793/11/be/src/runtime/runtime-filter-bank.h@78 PS11, Line 78: /// may both be NULL, representing a filter that allows all rows to pass. > is it the case that at most one of bloom_filter and min_max_filter should b Done http://gerrit.cloudera.org:8080/#/c/7793/11/be/src/util/min-max-filter.h File be/src/util/min-max-filter.h: http://gerrit.cloudera.org:8080/#/c/7793/11/be/src/util/min-max-filter.h@62 PS11, Line 62: Materialize filter values > what does that mean? Done http://gerrit.cloudera.org:8080/#/c/7793/9/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: http://gerrit.cloudera.org:8080/#/c/7793/9/common/thrift/PlanNodes.thrift@103 PS9, Line 103: 12: optional string kudu_col_name > case sensitive? Done http://gerrit.cloudera.org:8080/#/c/7793/11/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/7793/11/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@359 PS11, Line 359: public Operator getJoinOp() { return exprCmpOp_; } > getExprCmpOp() Done http://gerrit.cloudera.org:8080/#/c/7793/11/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@602 PS11, Line 602: if (!(targetExpr instanceof SlotRef) > I think only explicit casts are problematic. Implicit casts should be ok, o Right, we should be able to support integer implicit casts. The complication is that if, say, the calculated max is outside of the range for the type of the targeted column, we can't just pass that value into Kudu as it will complain. In that case, we would need to convert the max we send to be the max for the type of the targeted column. I've added some code to the BE to do this conversion. http://gerrit.cloudera.org:8080/#/c/7793/11/testdata/workloads/functional-query/queries/QueryTest/runtime_filters.test File testdata/workloads/functional-query/queries/QueryTest/runtime_filters.test: http://gerrit.cloudera.org:8080/#/c/7793/11/testdata/workloads/functional-query/queries/QueryTest/runtime_filters.test@144 PS11, Line 144: on a.month = cast(b.month + 10000 as int); > Was this your change? Why the change? This was necessary if we didn't support implicit casts on the target. Removed now. -- To view, visit http://gerrit.cloudera.org:8080/7793 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I02bad890f5b5f78388a3041bf38f89369b5e2f1c Gerrit-Change-Number: 7793 Gerrit-PatchSet: 11 Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Anonymous Coward #345 Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <mjac...@apache.org> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 07 Nov 2017 15:06:55 +0000 Gerrit-HasComments: Yes
