Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/8775 )
Change subject: IMPALA-4993: extend dictionary filtering to collections ...................................................................... Patch Set 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/8775/8/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: http://gerrit.cloudera.org:8080/#/c/8775/8/be/src/exec/hdfs-parquet-scanner.cc@778 PS8, Line 778: } > Makes sense. The function comment incorrectly states they are added to 'non fixed the comment-- yes, that was inconsistent. http://gerrit.cloudera.org:8080/#/c/8775/8/be/src/exec/hdfs-parquet-scanner.cc@809 PS8, Line 809: if (!state_->query_options().parquet_dictionary_filtering || dict_filter_map_.empty()) { > My point was about whether child readers are included or not. In this code clarified in the comment for the members. http://gerrit.cloudera.org:8080/#/c/8775/8/be/src/exec/hdfs-parquet-scanner.cc@845 PS8, Line 845: // where some data pages are dictionary-encoded and others are plain > Agree. Would be good to add a Preonditions check to the FE SlotDescriptor c added to FE's SlotDescriptor. http://gerrit.cloudera.org:8080/#/c/8775/8/be/src/exec/hdfs-scanner.h File be/src/exec/hdfs-scanner.h: http://gerrit.cloudera.org:8080/#/c/8775/8/be/src/exec/hdfs-scanner.h@222 PS8, Line 222: typedef std::map<SlotId, std::vector<ScalarExprEvaluator*>> > Makes sense. added to TupleDescriptor; its the one that currently has more of an overview. http://gerrit.cloudera.org:8080/#/c/8775/8/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/8775/8/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@548 PS8, Line 548: private boolean isCollectionNotEmpty(TupleDescriptor desc) { > I'm still in favor of removing. Reasons: makes sense. inlined it. http://gerrit.cloudera.org:8080/#/c/8775/8/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1102 PS8, Line 1102: // in the same order as the normal conjuncts. Sort the indices so that the > Sorry this was my being vague. I meant creating a helper function for addin Done http://gerrit.cloudera.org:8080/#/c/8775/8/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1116 PS8, Line 1116: for (Integer idx : totalIdxList) { > The perTupleConjuncts already have a new list, so no member is modified wou yup, completely missed that. http://gerrit.cloudera.org:8080/#/c/8775/9/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/8775/9/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@639 PS9, Line 639: List<TupleId> tupleIds = Lists.newArrayList(); > remove, Preconditions check in L646 must trivially be true after L645 Done http://gerrit.cloudera.org:8080/#/c/8775/8/testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test File testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test: http://gerrit.cloudera.org:8080/#/c/8775/8/testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test@330 PS8, Line 330: parquet statistics predicates: c_custkey > 0, o.o_orderkey > 0, l.l_partkey > 0 > that pattern makes more sense to me as well, so the oddity is really an exi added a jira to track it. -- To view, visit http://gerrit.cloudera.org:8080/8775 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If3a2abcfc3d0f7d18756816659fed77ce12668dd Gerrit-Change-Number: 8775 Gerrit-PatchSet: 9 Gerrit-Owner: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Fri, 12 Jan 2018 02:29:51 +0000 Gerrit-HasComments: Yes
