Dmitry Lychagin has posted comments on this change. Change subject: [NO ISSUE][FUN] Implement object-replace() ......................................................................
Patch Set 1: (6 comments) https://asterix-gerrit.ics.uci.edu/#/c/2708/1/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/objects/object_replace/object_replace.3.query.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/objects/object_replace/object_replace.3.query.sqlpp: Line 34: object_replace({"a":1}, {"b":2}, "z") is null, in n1ql object_replace({"a":1}, {"b":2}, "z") returns { "a": 1 } this is a deep equality case which we do not currently support. I talked to Till about it and we agreed that we should fail with a type mismatch error if 2nd argument is not a primitive type until we fully implement it. need to test it too, so probably need two negative testcases one where 2nd argument is an object another when it's an array. PS1, Line 35: object_replace({"a":1}, "b", ["z"]) object_replace({"a":1}, "b", ["z"]) is not a deep equality case and it is supposed to return {"a":1}. how come we return null? Line 37: ], let's to add testcases: 1) object_replace({"a":1}, 1, null) -- supposed to return {"a": null } 2) object_replace({"a":1, "b":1}, 1, 2); -- where more than on value matches https://asterix-gerrit.ics.uci.edu/#/c/2708/1/asterixdb/asterix-doc/src/main/markdown/builtins/8_record.md File asterixdb/asterix-doc/src/main/markdown/builtins/8_record.md: Line 368: * `old_value` : an atomic value to be replaced by `new_value`. we call these types primitive in the documentation, not atomic. (applies to the next line too) Line 374: * `null` if any argument is `null` or `input_object` is non-object value, or `old_value` is non-atomic value, or 1) (see my other comment on the testcase) if the old_value is not a primitive type then we need to fail. (until we implement deep-equality) 2) I don't see a problem with the new_value being a non-primitive type. why return null? https://asterix-gerrit.ics.uci.edu/#/c/2708/1/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/functions/FunctionCollection.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/functions/FunctionCollection.java: Line 652: fc.addGenerated(RecordReplaceDescriptor.FACTORY); it looks like this function cannot use byte-code generated runtime because the result is not necessarily null if the third argument is null. e.g. object_replace({"a":1}, 1, null) is supposed to return {"a": null } -- 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: 1 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: Till Westmann <[email protected]> Gerrit-HasComments: Yes
