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: (6 comments) 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@50 PS7, Line 50: > There seems to be nothing UUID-specifc in here. I renamed it to INSTANCE_IGNORE_DUPLICATES, it passes a true boolean flag to the constructor, that flag will ignore the duplicates. Please see the next comment. https://asterix-gerrit.ics.uci.edu/#/c/3370/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordMergeTypeComputer.java@52 PS7, Line 52: : private RecordMergeTypeComputer(boolean isIgnoreDuplicates) { : this.isIgnoreDuplicates = isIgnoreDuplicates; : } : : private final boolean isIgnoreDuplicates; : The first constructor maintains the old behavior, passes a false flag, the second constructor is the one used by the INSTANCE_IGNORE_DUPLICATES. I also renamed the isUuidIgnoreDuplicate to isIgnoreDuplicates, as it can be generalized safely. 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 { > where do we check types? Short answer: ARecordCaster is injected by one of the rules in the optimization plan, that does the checking. I should probably mention that in the comment though. Long answer: Say we have { "id": string } and { "id": uuid } in this case, the user is passing string for the id (should be uuid type). Now, if the user is just calling record-merge normally (isUuidIgnoreDuplicate is false), then the proper error is to throw a duplicate field name exception. However, if this call is coming from an introduced uuid that needs to be merged (isUuidIgnoreDuplicate is true), then duplicate field is not really the real issue, it's the incompatible type with the primary key type in the dataset, so we defer handling the error here, and let the ARecordCaster which is injected by another rule do the runtime validation and confirm that each field type matches the schema closed part. That will throw a type mismatch exception, that a string cannot be promoted to uuid. https://asterix-gerrit.ics.uci.edu/#/c/3370/4/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/4/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeEvaluator.java@162 PS4, Line 162: > One way to think about this could be to distinguish between "conflicting" a That makes sense ot me, done. https://asterix-gerrit.ics.uci.edu/#/c/3370/4/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeEvaluator.java@207 PS4, Line 207: > It looks like a private method in a new file - so I don't see the continuit You're right, done. https://asterix-gerrit.ics.uci.edu/#/c/3370/7/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeUuidIgnoreDuplicateDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeUuidIgnoreDuplicateDescriptor.java: https://asterix-gerrit.ics.uci.edu/#/c/3370/7/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeUuidIgnoreDuplicateDescriptor.java@43 PS7, Line 43: > RecordMergeIgnoreDuplicateFields or RecordMergeIgnoreConflictingFields? It Sounds about right, done. -- 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: Thu, 09 May 2019 11:56:33 +0000 Gerrit-HasComments: Yes
