impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vuk Ercegovac (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6286: Remove invalid runtime filter targets.
Date Fri, 08 Dec 2017 07:27:40 GMT
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.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <vercegovac@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2017 07:27:40 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message