Ali Alsuliman has posted comments on this change. Change subject: [ASTERIXDB-2516][RT] add support for deep comparison 1 ......................................................................
Patch Set 2: (18 comments) https://asterix-gerrit.ics.uci.edu/#/c/3145/1/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: PS1, Line 35: COMPARE, > COMPARE Done PS1, Line 36: ORDER > ORDER Done PS1, Line 39: asResult(int > "asResult" ? This is basically a cast from int to Result. Could also be a s Done PS1, Line 40: return result < 0 ? Result.LT : (result == 0 ? Result.EQ : Result.GT); : } : : Result compare(byte[] leftBytes, int leftStart, int leftLen, byte[] rightBytes, int rightStart, int rightLen) : throws HyracksDataException; : : Resul > return result < 0 ? Result.LT : (result == 0 ? Result.EQ : Result.GT) Done PS1, Line 50: ompare(IAObject lef > Could this be part of the factory and not a parameter for the comparison? Done 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 > just curious. If one is ANY and another is derived then it'll go to Logical I believe so. I'm checking the runtime tag. If ANY yielded a derived type, it would trigger the complex code. If ANY yielded something else, it would trigger the scalar which should return MISMATCH. I'll include this as one of the test cases later. Line 137 > we'll need to have a separate method for comparing integer values without c Done Line 155 > integer values (tinyint to bigint) should compared as integers (longs) with Done PS1, Line 157: > Could we switch on the type tag instead of comparing the object(-reference) Done 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 final IBinaryComparator uuidBinaryComparator = > may be rename to INEQUALITY_UNDEFINED_TYPES, to make it clear that it's onl Done Line 111: > unrelated to this particular change. we should use AYearMonthDurationSerial Done Line 119: break; > same comment as above. we should use ADayTimeDurationSerializerDeserializer Done Line 199: public Result compare(IAObject leftConstant, byte[] rightBytes, int rightStart, int rightLen) { > we need to double check whether it can happen or not. If it is not supposed Ok. I removed it. https://asterix-gerrit.ics.uci.edu/#/c/3145/1/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/AbstractComparisonEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/AbstractComparisonEvaluator.java: PS1, Line 109: // both are constants : return logicalComparator.compare(leftConstant, rightConstant); : } else { : // left is constant, right isn't : return logicalComparator.compare(leftConstant, argRight.getByteArray(), argRight.getStartOffset(), : argRight.getLength()); : } : } else { : if (rightConstant != null) { : // right is constant, left isn't : return logicalComparator.compare(argLeft.getByteArray(), argLeft.getStartOffset(), argLeft.getLength(), : rightConstant); : } else { : return logicalComparator.compare(argLeft.getByteArray(), argLeft.getStartOffset(), argLeft.getLength(), : > if (leftConstant != null) { Done https://asterix-gerrit.ics.uci.edu/#/c/3145/1/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/AbstractIfEqualsEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/AbstractIfEqualsEvaluator.java: PS1, Line 41: case MISSING: : writeMissing(result); : break; : case NULL: : writeNull(result); : break; : case EQ: : resultStorage.reset(); : writeEqualsResult(); : result.set(resultStorage); : break; : > switch(compare()) { Done. That's exactly why I had to introduce MISMATCH. Normally, incompatible types would return NULL, but this function returns the left argument if there is a mismatch (of course, it returns the left arg if they are not equal, as well). 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: switch (comparisonResult) { > minor. switch () will be slightly faster (one comparison instead of 3 here) Done https://asterix-gerrit.ics.uci.edu/#/c/3145/1/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/EqualsDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/EqualsDescriptor.java: PS1, Line 39: @Override : public IFunctionDescr > pull-up into superclass? You mean creating an abstract class? Done. But not sure if this will make Mike happy now he is dealing with serialization :) It should be fine, but I'll just loop him in just in case. PS1, Line 60: @Override : public IScalarEvaluator createScalarEvaluator(IHyracksTaskContext ctx) throws HyracksDataException { : return new AbstractValueComparisonEvaluator(args[0], leftType, args[1], rightType, ctx, sourceLoc, : > pull-up into superclass? Done -- 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: 2 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Reviewer: 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
