Dmitry Lychagin has posted comments on this change. Change subject: [NO ISSUE][FUN] Implement object-replace() ......................................................................
Patch Set 5: (2 comments) https://asterix-gerrit.ics.uci.edu/#/c/2708/5/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/ComparisonHelper.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/ComparisonHelper.java: Line 468: return INCOMPATIBLE_TYPES_COMPARISION_RESULT; This might be error-prone because ComparisonHelper could be used for <, > operations too, not just =. any result value < 0 means "less", not "incompatible" types. so there could be collisions with -2. I don't think you need to modify this class at all. Other evaluators that use ComparisonHelper first check whether types are compatible by calling ATypeHierarchy.isCompatible() and if they're then they call ComparisonHelper.compare(). See AbstractComparisonEvaluator.comparabilityCheck(). RecordReplaceEvaluator should do the same. https://asterix-gerrit.ics.uci.edu/#/c/2708/5/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordReplaceEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordReplaceEvaluator.java: Line 121: if (comparisonHelper.compare(existingValueTypeTag, oldValueTypeTag, existingValuePtr, oldValuePtr) != 0) { first call ATypeHierarchy.isCompatible() and only then comparisonHelper.compare(). See my other comment. -- To view, visit https://asterix-gerrit.ics.uci.edu/2708 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2907f827a1dc5bb35f340bfd25d51e1fdd6fde20 Gerrit-PatchSet: 5 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
