date:20190510

Change in asterixdb[master]: [ASTERIXDB-2535][COMP] Fix uuid present in insert/upsert statement

2019-05-10 Thread Ali Alsuliman (Code Review)

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:

(1 comment)

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 agree with you. But my concern is, if a user (say by mistake) tries input
You should think of both merge functions as standalone functions. They could
appear anywhere in any query. There is no guarantee that casting will be
injected. Casting is not specific to insert/upsert. If you are worried about
the error message being confusing for uuid case, there are workarounds. But we
shouldn't change the generic case to fit the special case.

--
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
Gerrit-Reviewer: Ali Alsuliman
Gerrit-Reviewer: Anon. E. Moose (1000171)
Gerrit-Reviewer: Dmitry Lychagin
Gerrit-Reviewer: Hussain Towaileb
Gerrit-Reviewer: Hussain Towaileb
Gerrit-Reviewer: Jenkins
Gerrit-Reviewer: Till Westmann
Gerrit-Comment-Date: Sat, 11 May 2019 05:30:27 +
Gerrit-HasComments: Yes

Change in asterixdb[master]: [NO ISSUE][FUN] Add ARRAY_AGG() SQL aggregate function

2019-05-10 Thread Anon. E. Moose (Code Review)

Anon. E. Moose (1000171) has posted comments on this change. ( 
https://asterix-gerrit.ics.uci.edu/3390 )

Change subject: [NO ISSUE][FUN] Add ARRAY_AGG() SQL aggregate function
..


Patch Set 4: Contrib+1

Analytics Compatibility Tests Successful
https://cbjenkins.page.link/AfP4tXy2LxN9x97K6 : SUCCESS


--
To view, visit https://asterix-gerrit.ics.uci.edu/3390
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I704e031a1252493e83ad8d45c38b75e0b15c1896
Gerrit-Change-Number: 3390
Gerrit-PatchSet: 4
Gerrit-Owner: Dmitry Lychagin 
Gerrit-Reviewer: Anon. E. Moose (1000171)
Gerrit-Reviewer: Jenkins 
Gerrit-Comment-Date: Sat, 11 May 2019 04:24:53 +
Gerrit-HasComments: No

Change in asterixdb[master]: [ASTERIXDB-2535][COMP] Fix uuid present in insert/upsert statement

2019-05-10 Thread Hussain Towaileb (Code Review)

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.

Change in asterixdb[master]: [NO ISSUE][FUN] Add ARRAY_AGG() SQL aggregate function

2019-05-10 Thread Anon. E. Moose (Code Review)

Anon. E. Moose (1000171) has posted comments on this change. ( 
https://asterix-gerrit.ics.uci.edu/3390 )

Change subject: [NO ISSUE][FUN] Add ARRAY_AGG() SQL aggregate function
..


Patch Set 4: Contrib-2

Analytics Compatibility Tests Failed
https://cbjenkins.page.link/YEUbpjEp2nV1fL449 : UNSTABLE


--
To view, visit https://asterix-gerrit.ics.uci.edu/3390
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I704e031a1252493e83ad8d45c38b75e0b15c1896
Gerrit-Change-Number: 3390
Gerrit-PatchSet: 4
Gerrit-Owner: Dmitry Lychagin 
Gerrit-Reviewer: Anon. E. Moose (1000171)
Gerrit-Reviewer: Jenkins 
Gerrit-Comment-Date: Sat, 11 May 2019 01:10:57 +
Gerrit-HasComments: No

34 matches

Mail list logo