Hussain Towaileb 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: (7 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 f You're right, I'm gonna make a follow up change shortly for record merge ignore duplicates and add test cases. (probably everything else is set) 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 { > I think relying on ARecordCaster being injected isn't a good idea. We shoul I agree with you. But my concern is, if a user (say by mistake) tries inputting a string uuid {id: "12345"} instead of {id: uuid("12345")}, telling him he's having a duplicate field issue won't make much sense, we need somehow to clarify that his issue is with the value being input. For example, if I do: insert into test([{"id": "12345", "comment": "my new record"}]); And you tell me I got a duplicate field issue, it won't make sense to me, what duplicate fields? (Users aren't aware that we're doing record merging under the hood to add the uuid field for them). But I do agree with you on the above completely. I think I should check if the ARecordCaster's job is to get injected for INSERT/UPSERT statements just to do this task, if that's the sole purpose of that rule, then it makes sense to leave the job for it. I'll also have to see what its type computer is doing, maybe it can detect those issue during compile time, not just runtime. But then again, we'll have to see if we should let different field types pass during record-merge-ignore-duplicates, or we should always stop them and throw an exception. I guess my original understanding of the ignore duplicates was to take the left one, ignoring whatever type and value exists for the right one, but I get your point. Will go through it more and see. 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) Done. (Change not submitted yet) https://asterix-gerrit.ics.uci.edu/#/c/3370/8/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeEvaluator.java: https://asterix-gerrit.ics.uci.edu/#/c/3370/8/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeEvaluator.java@64 PS8, Line 64: @MissingNullInOutFunction Removed. (change not submitted yet) Thanks for noticing that @Ali. 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 eva It is for Descriptors only. 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. Done. (Change not submitted yet) https://asterix-gerrit.ics.uci.edu/#/c/3370/8/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeIgnoreDuplicatesEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeIgnoreDuplicatesEvaluator.java: https://asterix-gerrit.ics.uci.edu/#/c/3370/8/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeIgnoreDuplicatesEvaluator.java@35 PS8, Line 35: @MissingNullInOutFunction Removed. (Change not submitted yet) Thanks for noticing that @Ali. -- 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 <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose (1000171) Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Comment-Date: Sat, 11 May 2019 03:24:50 +0000 Gerrit-HasComments: Yes
