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 http://gerrit.cloudera.org:8080/8014 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5bd0aa82a0316ac236b7ecf8612ef4b30c016a00 Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Vuk Ercegovac <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-HasComments: No
