Alex Behm 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) 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 The predicates generated in this function are for optimization purposes and not required for query correctness. We should be able to tolerate the failure of such non-essential expr evaluations. 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. If we switch to an enum then the exception cause it truly swallowed. I prefer to throw to preserve the root cause. The caller is free to not eat the exception or log. Some callers of this function do not swallow the exception 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 no Correct. * The previous Preconditions check was triggered due to a low mem limit. * Most callers of this function are trying to perform an optimization that is not required for query correctness. In those cases we might still be able to run the query if we ignore the exception and the optimization. I don't think the mem-limit test should be changed because we should be able to tolerate failures of such non-essential expr evaluations without failing the query. * One notable caller is TupleIsNullPredicate.requiresNullWrapping() which requires the evaluation to succeed for query correctness. That caller forwards the exception and will fail the query. * I added LOG.warn() at the non-essential call sites. 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 Added LOG.warn() here and in other non-essential callers. -- 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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 08 Dec 2017 17:03:40 +0000 Gerrit-HasComments: Yes
