Dmitry Lychagin has posted comments on this change. Change subject: [ASTERIXDB-2516][RT] add support for deep comparison 1 ......................................................................
Patch Set 1: (8 comments) https://asterix-gerrit.ics.uci.edu/#/c/3145/1/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/common/LogicalComparatorUtil.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/common/LogicalComparatorUtil.java: Line 61: return new LogicalGenericBinaryComparator(leftType, rightType); just curious. If one is ANY and another is derived then it'll go to LogicalGenericBinaryComparator. Will it be able to handle this case? Line 137: private static double getValue(ATypeTag numericTag, byte[] b, int s) { we'll need to have a separate method for comparing integer values without converting them to double and use that if both arguments are integers (from tinyint to bigint) Line 155: private static double getConstantValue(IAObject numeric) { integer values (tinyint to bigint) should compared as integers (longs) without converting them to double. https://asterix-gerrit.ics.uci.edu/#/c/3145/1/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/LogicalScalarBinaryComparator.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/LogicalScalarBinaryComparator.java: Line 70: private static final EnumSet<ATypeTag> UNDEFINED_TYPES = may be rename to INEQUALITY_UNDEFINED_TYPES, to make it clear that it's only about inequality Line 111: case YEARMONTHDURATION: unrelated to this particular change. we should use AYearMonthDurationSerializerDeserializer.getYearMonth() instead of AInt32SerializerDeserializer.getInt() here. Same for other types: ATimeSerializerDeserializer/ADateSerializerDeserializer.getChronon(), Line 119: result = Long.compare(AInt64SerializerDeserializer.getLong(leftBytes, leftStart), same comment as above. we should use ADayTimeDurationSerializerDeserializer.getDayTime(), ADateTimeSerializerDeserializer.getChronon() Line 199: // illegal state maybe??? we need to double check whether it can happen or not. If it is not supposed to happen then either throw IllegalState or just remove this if statement and let the next two lines throw NPE. https://asterix-gerrit.ics.uci.edu/#/c/3145/1/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/AbstractValueComparisonEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/AbstractValueComparisonEvaluator.java: Line 49: if (comparisonResult == Result.MISSING) { minor. switch () will be slightly faster (one comparison instead of 3 here) -- 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: 1 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <dmitry.lycha...@couchbase.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-HasComments: Yes
