impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Date Wed, 13 Sep 2017 20:49:30 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.

Patch Set 8:

Vuk, I have concerns regarding the high-level approach. From a bird's eye view this patch
adds a lot of new code and complexity for a simple new function.

I think the simplest (and preferable) implementation would look like this:
1. Unify the new expr with the existing IsNullPredicate; the new expr provides a super set
of functionality
2. Have a proper BE implementation; the function should have 3 arguments: one is the input
expr, another is a boolean indicating negation and the third is an int indicating the kind
of check we are doing (checking for isnull/isfalse/istrue). That way we can codegen away the
runtime overhead of checking those cases.

The current solution takes the approach of BetweenPredicate which is tricky and error prone.
For example, in your patch you forget to add the new rewrite rule into the HdfsPartitionPruner.
Overall it's tricky to find all those places, and In think that eventually we may want to
change how BetweenPredicate works as well.

What do you think? Happy to discuss.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Thomas Tauber-Marshall <>
Gerrit-Reviewer: Vuk Ercegovac <>
Gerrit-HasComments: No

View raw message