Dmitry Lychagin has posted comments on this change. Change subject: [NO ISSUE][FUN] Implement object-replace() ......................................................................
Patch Set 4: (6 comments) https://asterix-gerrit.ics.uci.edu/#/c/2708/4/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/objects/object_replace/object_replace.4.query.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/objects/object_replace/object_replace.4.query.sqlpp: Line 22: * Expected Res : Success Expected Res should be "Failure". https://asterix-gerrit.ics.uci.edu/#/c/2708/4/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/objects/object_replace/object_replace.5.query.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/objects/object_replace/object_replace.5.query.sqlpp: Line 22: * Expected Res : Success Expected Res should be "Failure" https://asterix-gerrit.ics.uci.edu/#/c/2708/4/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/types/ATypeTag.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/types/ATypeTag.java: Line 71: PRIMITIVE(43); let's not introduce a new type tag just for error reporting purposes. (all type tags should correspond to types in the type system) We can instead change TypeMistmachException constructor to accept a String or String[] for expected type names and pass "primitive" there. https://asterix-gerrit.ics.uci.edu/#/c/2708/4/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 94: if (oldValueType == ATypeTag.OBJECT || oldValueType == ATypeTag.ARRAY || oldValueType == ATypeTag.MULTISET) { you could use oldValueType.isDerived() here Line 100: if (newValueType == ATypeTag.OBJECT) { or an array/multiset because an array/multiset can contain objects too. use newValueType.isDerved() here. Line 117: if (binaryComparator.compare(fieldValue.getByteArray(), fieldValue.getStartOffset(), fieldValue.getLength(), using RawBinaryComparatorFactory might be a problem here for comparing different numeric types (bigint and double, for instance). Can we add a testcase: object_replace({"a":1}, 1.0, 2). It is supposed to perform the replacement and return {"a": 2}. What do we return? -- 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: 4 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
