Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8623 )
Change subject: IMPALA-6187: Fix missing conjuncts evaluation with empty projection ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/8623/1/be/src/exec/hdfs-scanner.h File be/src/exec/hdfs-scanner.h: http://gerrit.cloudera.org:8080/#/c/8623/1/be/src/exec/hdfs-scanner.h@333 PS1, Line 333: If template tuple is not NULL > what if the template_tuple_ is null? Doesn't it effectively set it to that Actually, reading the old code, we seem to leave those tuple ptrs alone so they are most likely random values as we used malloc() to allocate the tuple_ptrs_ buffer in the RowBatch constructor. May be I misunderstood it. I switched to using calloc() in the constructor. Someone more familiar with the intricacy of null tuple pointer should see if it makes sense or flags it as unnecessary. http://gerrit.cloudera.org:8080/#/c/8623/1/be/src/exec/hdfs-scanner.h@337 PS1, Line 337: WriteEmptyProjection( > this has the same name as the KuduScanner method, but they are a bit differ Renamed the function. http://gerrit.cloudera.org:8080/#/c/8623/1/be/src/exec/hdfs-scanner.cc File be/src/exec/hdfs-scanner.cc: http://gerrit.cloudera.org:8080/#/c/8623/1/be/src/exec/hdfs-scanner.cc@215 PS1, Line 215: > must, or "which don't reference any slots" Rephrased to "should reference partition columns only if any." http://gerrit.cloudera.org:8080/#/c/8623/1/be/src/exec/hdfs-scanner.cc@215 PS1, Line 215: > I'm wondering whether this assertion and the code is correct. What if we ha Good point. Fixed in the new patch. Also added a new test case. http://gerrit.cloudera.org:8080/#/c/8623/1/testdata/workloads/functional-query/queries/QueryTest/scanners.test File testdata/workloads/functional-query/queries/QueryTest/scanners.test: http://gerrit.cloudera.org:8080/#/c/8623/1/testdata/workloads/functional-query/queries/QueryTest/scanners.test@80 PS1, Line 80: select count(*) from alltypes where rand() * 10 >= 0.0; > I think rand() can return 0.0, so maybe it would be better to make this >= Done -- To view, visit http://gerrit.cloudera.org:8080/8623 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib530f1fdcd2c6de699977db163b3f6eb38481517 Gerrit-Change-Number: 8623 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 28 Nov 2017 02:31:50 +0000 Gerrit-HasComments: Yes
