Alex Behm has posted comments on this change. Change subject: IMPALA-3856,IMPALA-3871: Fix BinaryPredicate normalization for Kudu ......................................................................
Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/3986/1/fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java File fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java: Line 126: SlotRef ref = predicate.getBoundSlot(); I think we should make this procedure more specific. getBoundSlot() internally uses unwrapSlotRef() which drills through implicit casts, but we don't actually want that here. Imo, we should check instanceof SlotRef on the children and not rely on getBoundSlot() or unwrapSlotRef() Line 135: predicate.foldConstantChildren(analyzer); Some of the functions used here feel a little clunky with respect to exception behavior. It's weird that this function throws for unsupported literal types. I think we should change foldConstantChildren() in one of the following ways: * either not throw at all and just leave exprs that failed to fold * only throw when the BE eval failed and add a boolean return value indicating whether all exprs succeeded in folding or not Happy to discuss if you have other ideas for cleaning this up. http://gerrit.cloudera.org:8080/#/c/3986/1/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: Line 100: def test_kudu_expr(self, vector): might be safer to mark this as serial due to setup and teardown. I know it's not required if only this one test is is not run serially, but as soon as you add another it's going to break. -- To view, visit http://gerrit.cloudera.org:8080/3986 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iae7612433a2e27f8887abe6624f9ee0f4867b934 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-HasComments: Yes
