Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/8783 )
Change subject: IMPALA-6286: Remove invalid runtime filter targets. ...................................................................... Patch Set 6: (4 comments) I have some questions on the different handling of that exception and a suggestion to make the callsites more explicit. http://gerrit.cloudera.org:8080/#/c/8783/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/8783/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1533 PS6, Line 1533: isTrueWithNullSlots prior, this would have tripped an assertion on backend error. why is it ok to ignore now? http://gerrit.cloudera.org:8080/#/c/8783/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1909 PS6, Line 1909: boolean might be clearer to have an enum here: true, false, unknown. instead, you are eating the error-- which presumably is rare and we may want to know about it-- and folding the unknown case into every call-site's catch. http://gerrit.cloudera.org:8080/#/c/8783/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1923 PS6, Line 1923: return FeSupport.EvalPredicate(nullTuplePred, getQueryCtx()); just noting that prior, we explicitly asserted the exception case should not happen whereas now that case is silently eaten (L1535). is this what was tripped by the test's too low limit? if so, shouldn't the tests be modified? that test includes an is null predicate for the nested collection-- unclear if that's needed for the test. http://gerrit.cloudera.org:8080/#/c/8783/6/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/8783/6/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@640 PS6, Line 640: continue would it be useful to know that pruning could not be done due to a backend error? -- To view, visit http://gerrit.cloudera.org:8080/8783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I88153eea9f4b5117df60366fad2bd91776b95298 Gerrit-Change-Number: 8783 Gerrit-PatchSet: 6 Gerrit-Owner: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Fri, 08 Dec 2017 07:27:40 +0000 Gerrit-HasComments: Yes
