Ali Alsuliman has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/3370 )
Change subject: [ASTERIXDB-2535][COMP] Fix uuid present in insert/upsert statement ...................................................................... Patch Set 8: (5 comments) https://asterix-gerrit.ics.uci.edu/#/c/3370/8/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/functions/BuiltinFunctions.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/functions/BuiltinFunctions.java: https://asterix-gerrit.ics.uci.edu/#/c/3370/8/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/functions/BuiltinFunctions.java@2186 PS8, Line 2186: addFunction(RECORD_MERGE_IGNORE_DUPLICATES, RecordMergeTypeComputer.INSTANCE_IGNORE_DUPLICATES, true); with this now being a new function and not related to UUID, we should add few test cases to make sure you are covered. You could add them in a later change. https://asterix-gerrit.ics.uci.edu/#/c/3370/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordMergeTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordMergeTypeComputer.java: https://asterix-gerrit.ics.uci.edu/#/c/3370/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordMergeTypeComputer.java@115 PS7, Line 115: : else { > Short answer: I think relying on ARecordCaster being injected isn't a good idea. We should maintain that the duplicate fields must have the same types. That's at compile-time here in the type computer. At runtime, you get to choose what to do for duplicates; whether to 1) throw duplicate exception when left and right have different types (normal current behaviour) (actually it throws exception when they have different values, i.e. if "id": 2, "id": 3. For "id":2, "id":2, it picks left) 2) ignore and pick the one on left input record (but also making sure left type is the same as the corresponding field in output record. This will be consistent with the type computer since it's picking the left type) In your example, if the "id" is part of the output record schema, then we should check that the input value type is the same as the one in the schema. The semantics of the record-merge-ignore-duplicates "runtime" says if there are duplicate fields, it is going to pick the left value (that's also the semantics of normal record merge for same-value duplicates that the compiler couldn't figure out because they are open fields). https://asterix-gerrit.ics.uci.edu/#/c/3370/8/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordMergeTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordMergeTypeComputer.java: https://asterix-gerrit.ics.uci.edu/#/c/3370/8/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordMergeTypeComputer.java@49 PS8, Line 49: private RecordMergeTypeComputer() { : this(false); : } : : private RecordMergeTypeComputer(boolean isIgnoreDuplicates) { : this.isIgnoreDuplicates = isIgnoreDuplicates; : } it's better to have one constructor with a boolean parameter) https://asterix-gerrit.ics.uci.edu/#/c/3370/8/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeIgnoreDuplicatesDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeIgnoreDuplicatesDescriptor.java: https://asterix-gerrit.ics.uci.edu/#/c/3370/8/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeIgnoreDuplicatesDescriptor.java@36 PS8, Line 36: @MissingNullInOutFunction just to make sure; is this annotation supposed to be for descriptors or evaluators? https://asterix-gerrit.ics.uci.edu/#/c/3370/8/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeIgnoreDuplicatesDescriptor.java@68 PS8, Line 68: RecordMergeIgnoreDuplicatesEvaluator we could remove this class and just extend RecordMergeEvaluator right here. -- To view, visit https://asterix-gerrit.ics.uci.edu/3370 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22100d3ff29864b8bfd54b0decb183e5056fdb4a Gerrit-Change-Number: 3370 Gerrit-PatchSet: 8 Gerrit-Owner: Hussain Towaileb <hussai...@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: Hussain Towaileb <hussai...@gmail.com> Gerrit-Reviewer: Hussain Towaileb <hussai...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Comment-Date: Fri, 10 May 2019 19:55:33 +0000 Gerrit-HasComments: Yes
