Dmitry Lychagin has posted comments on this change. Change subject: [ASTERIXDB-2516][RT] add support for deep comparison 1 ......................................................................
Patch Set 6: Code-Review+1 (2 comments) https://asterix-gerrit.ics.uci.edu/#/c/3145/6/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/common/ILogicalBinaryComparator.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/common/ILogicalBinaryComparator.java: Line 34: enum Operation { minor, but both names seem to be confusing. COMPARE here means EQ only, which is not obvious. ORDER seems to imply that this operation might be used by ORDER BY, but it won't. I'd call them EQ / LT, or EQ / EQ_LT_GT or even have a boolean equalsOnly = true / false. We can address this in the next change though, as this is not critical. https://asterix-gerrit.ics.uci.edu/#/c/3145/6/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/LogicalComparatorUtil.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/LogicalComparatorUtil.java: Line 156: default: minor. I'd use "case DOUBLE" here instead of 'default' and throw some exception in "default". I know the code above ensures that only these numeric types reach here, but an additional check will make it future-proof. This applies to all other switch statement below. -- To view, visit https://asterix-gerrit.ics.uci.edu/3145 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I623662861e6f3b1fdcff78f8edc0e3216ca10fe1 Gerrit-PatchSet: 6 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
