You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@asterixdb.apache.org by AsterixDB Code Review <do...@asterix-gerrit.ics.uci.edu> on 2022/08/09 20:42:08 UTC
Change in asterixdb[master]: [COMP][ASTERIXDB-3041] Index usage w/ OBJECT_* functions.
From Glenn Galvizo <gg...@uci.edu>:
Glenn Galvizo has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843 )
Change subject: [COMP][ASTERIXDB-3041] Index usage w/ OBJECT_* functions.
......................................................................
Patch Set 5:
(21 comments)
I added a few tests that toy around closed vs. open types. There are already some tests for OBJECT_ADD / OBJECT_REMOVE / OBJECT_PUT / OBJECT_RENAME that test MISSING and NULL behavior (see the objects/* test folder).
We will always overwrite the record field type with the type of the new value for OBJECT_ADD / OBJECT_PUT.
> Patch Set 5:
>
> (19 comments)
>
> - we should add runtime tests to cover the different scenarios especially with NULL/MISSING involved.
> - OBJECT_ADD() and OBJECT_PUT() are a bit tricky. I'm still thinking about the
> closed form vs open form of a value especially for derived types (lists and records).
> e.g. can it happen that we are adding a list value in its open form (3rd arg)
> in the closed part of produced output record?
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordAddTypeComputer.java
File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordAddTypeComputer.java:
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordAddTypeComputer.java@56
PS5, Line 56: if (type0.getTypeTag() == ATypeTag.ANY) {
> you can extract type0.getTypeTag() up into ATypeTag tag0 = type0.getTypeTag() and use it everywhere.
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordAddTypeComputer.java@57
PS5, Line 57: return AUnionType.createUnknownableType(DefaultOpenFieldType.NESTED_OPEN_RECORD_TYPE,
> extra line below the return (same for the other returns)
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordAddTypeComputer.java@64
PS5, Line 64: } else if (type0.getTypeTag() != ATypeTag.OBJECT || inputRecordType == null) {
> is "inputRecordType == null" check needed? if not, we can remove it and move line 55 below the if bl […]
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordAddTypeComputer.java@91
PS5, Line 91: return type0;
> type0 is not the original type of arg0 since type0 refers to the actual type (arg0 might be UNION(ob […]
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordAddTypeComputer.java@114
PS5, Line 114: fieldTypes[currentCursorPosition] = type2;
> shouldn't we keep the original type of arg2?
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordPutTypeComputer.java
File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordPutTypeComputer.java:
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordPutTypeComputer.java@53
PS5, Line 53:
> is there some way to deduplicate line 53-67 between these type computers? it seems very similar
made a parent class to contain the common parts of RECORD_ADD, RECORD_PUT, RECORD_RENAME, and RECORD_REMOVE
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordPutTypeComputer.java@57
PS5, Line 57: return AUnionType.createUnknownableType(DefaultOpenFieldType.NESTED_OPEN_RECORD_TYPE,
> extra line below the return (same for the other returns and the if-else blocks)
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordPutTypeComputer.java@102
PS5, Line 102: boolean wasFieldFound = false;
> we could rename it to fieldFound
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordPutTypeComputer.java@103
PS5, Line 103: ArrayList<String> outputFieldNames = new ArrayList<>();
> the practice around here is to use List<String> (the interface) for the variable :)
whoops! was using an iterator before and forgot to change it back, thanks :-)
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordPutTypeComputer.java@117
PS5, Line 117: outputFieldTypes.add(type2);
> should we keep the original type of arg2?
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordPutTypeComputer.java@140
PS5, Line 140: outputFieldTypes.add(type2);
> should we keep the original type of arg2?
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordRemoveTypeComputer.java
File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordRemoveTypeComputer.java:
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordRemoveTypeComputer.java@57
PS5, Line 57: return AUnionType.createUnknownableType(DefaultOpenFieldType.NESTED_OPEN_RECORD_TYPE,
> extra line below the return (same for the other returns)
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordRenameTypeComputer.java
File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordRenameTypeComputer.java:
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordRenameTypeComputer.java@59
PS5, Line 59: return AUnionType.createUnknownableType(DefaultOpenFieldType.NESTED_OPEN_RECORD_TYPE,
> extra line below the return (same for the other returns and the if-else blocks)
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordRenameTypeComputer.java@93
PS5, Line 93: // We cannot infer the type of our second argument-- our output may be MISSING or NULL.
> third
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordRenameTypeComputer.java@120
PS5, Line 120: Mutable<Boolean> wasFieldFound = new MutableObject<>(false);
> we could rename it to fieldFound
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/AbstractRecordAddPutEvaluator.java
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/AbstractRecordAddPutEvaluator.java:
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/AbstractRecordAddPutEvaluator.java@50
PS5, Line 50: protected final UTF8StringPointable newFieldNamePointable = new UTF8StringPointable();
> i think we should not use UTF8StringPointable. […]
got it, using a void-pointable instead
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/AbstractRecordAddPutEvaluator.java@69
PS5, Line 69: this.inputRecordPointable = new PointableAllocator().allocateRecordValue(inRecType);
> extra line below (and in some other places down)
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/AbstractRecordAddPutEvaluator.java@72
PS5, Line 72: // We should only enter this branch in the ConstantFoldingRule, where we don't set our immutable states.
> i think we do set the immutables in ConstantFoldingRule. […]
moved this logic closer to where we actually use the inputRecordPointable
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/AbstractRecordAddPutEvaluator.java@83
PS5, Line 83: if (PointableHelper.getTypeTag(inputPointable) == ATypeTag.MISSING
> let's extract both PointableHelper.getTypeTag(inputPointable) and PointableHelper. […]
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordRenameEvaluator.java
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordRenameEvaluator.java:
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordRenameEvaluator.java@68
PS5, Line 68: this.outRecType = outRecType;
> these parts of the constructors seem almost common, not sure if there's a way to dedupe it
Ack
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843/5/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordRenameEvaluator.java@70
PS5, Line 70: this.inputRecordPointable = new PointableAllocator().allocateRecordValue(inRecType);
> extra line below (and in some other places down)
Ack
--
To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/16843
To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Change-Id: I2c64319981f504736cea8732b3ec985d5c913dd3
Gerrit-Change-Number: 16843
Gerrit-PatchSet: 5
Gerrit-Owner: Glenn Galvizo <gg...@uci.edu>
Gerrit-Reviewer: Ali Alsuliman <al...@gmail.com>
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Ian Maxon <im...@uci.edu>
Gerrit-Reviewer: Jenkins <je...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Wail Alkowaileet
Gerrit-Comment-Date: Tue, 09 Aug 2022 20:42:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Ian Maxon <im...@uci.edu>
Comment-In-Reply-To: Ali Alsuliman <al...@gmail.com>
Gerrit-MessageType: comment