Change in asterixdb[master]: Clean-up - Made record-merge visible from AQL + more proper ...

["Heri Ramampiaro (Code Review)" <do...@asterix-gerrit.ics.uci.edu>]

Heri Ramampiaro has posted comments on this change.

Change subject: Clean-up - Made record-merge visible from AQL + more proper dealing with nested records in record merge type computer
......................................................................


Patch Set 7:

(16 comments)

I revisited Preston's comments and did most of the changes accordingly. Please see below for more information... I will commit my changes for review once the tests are passed...

Thanks,
-heri

https://asterix-gerrit.ics.uci.edu/#/c/298/7/asterix-doc/src/site/markdown/aql/functions.md
File asterix-doc/src/site/markdown/aql/functions.md:

Line 375: ### uppercase ###
> Is uppercase listed twice in our documentation?
Done


Line 2232: 
> Can you add a better/more explaination on the idea of open status?
Done


Line 2309:           "address":{"state":"CA"}, 
> Does the function change the order of fields? If so, note that otherwise up
Done


Line 2548:  * Arguments:
> Do think we should make some notes here about what equality means? Based on
Done


https://asterix-gerrit.ics.uci.edu/#/c/298/7/asterix-metadata/src/main/java/edu/uci/ics/asterix/metadata/bootstrap/MetadataBootstrap.java
File asterix-metadata/src/main/java/edu/uci/ics/asterix/metadata/bootstrap/MetadataBootstrap.java:

Line 19
> Has this file been changed? If not, maybe remove this change.
Done


https://asterix-gerrit.ics.uci.edu/#/c/298/7/asterix-om/src/main/java/edu/uci/ics/asterix/om/typecomputer/impl/AbstractRecordManipulationTypeComputer.java
File asterix-om/src/main/java/edu/uci/ics/asterix/om/typecomputer/impl/AbstractRecordManipulationTypeComputer.java:

Line 49:     private final Deque<List<IAType>> aTypelistPool = new ArrayDeque<>();
> What about using a ListObjectPool object here?
I am using the Deque as a "buffer" only for the list ArrayList to restrict creating too many objects (with dynamic allocation and deallocation during runtime). I am afraid "ListObjectPool" would not cover the needs here, without adding similar methods anyway...


https://asterix-gerrit.ics.uci.edu/#/c/298/7/asterix-om/src/main/java/edu/uci/ics/asterix/om/typecomputer/impl/RecordMergeTypeComputer.java
File asterix-om/src/main/java/edu/uci/ics/asterix/om/typecomputer/impl/RecordMergeTypeComputer.java:

Line 134
> Is this used in other places? Why move this to abstract class?
Currently no, but it will be used in the next version of record-add-fields (to support nested fields).


https://asterix-gerrit.ics.uci.edu/#/c/298/7/asterix-runtime/src/main/java/edu/uci/ics/asterix/runtime/evaluators/comparisons/DeepEqualAssessor.java
File asterix-runtime/src/main/java/edu/uci/ics/asterix/runtime/evaluators/comparisons/DeepEqualAssessor.java:

Line 54:         if (PointableUtils.getTypeTag(vp0) != PointableUtils.getTypeTag(vp1))
> Add a note about only checking matching types, not equivalence.
Done


https://asterix-gerrit.ics.uci.edu/#/c/298/7/asterix-runtime/src/main/java/edu/uci/ics/asterix/runtime/evaluators/functions/DeepEqualityDescriptor.java
File asterix-runtime/src/main/java/edu/uci/ics/asterix/runtime/evaluators/functions/DeepEqualityDescriptor.java:

Line 64:             private ISerializerDeserializer boolSerde = AqlSerializerDeserializerProvider.INSTANCE
> can this be final?
Done


Line 84:                             Pair<IVisitablePointable, Boolean>(accessor1, Boolean.FALSE);
> Why Boolean over boolean?
Pair takes an input of an Object and not a primitive object thereby Boolean. Nevertheless I changed the value to "false" instead of Boolean.FALSE (though they are the same)...


https://asterix-gerrit.ics.uci.edu/#/c/298/7/asterix-runtime/src/main/java/edu/uci/ics/asterix/runtime/evaluators/functions/PointableUtils.java
File asterix-runtime/src/main/java/edu/uci/ics/asterix/runtime/evaluators/functions/PointableUtils.java:

Line 160:     public static UTF8StringPointable serializeString(String str, IMutableValueStorage vs) throws AlgebricksException {
> Would this function be better to pass the returned pointable as a parameter
Done


https://asterix-gerrit.ics.uci.edu/#/c/298/7/asterix-runtime/src/main/java/edu/uci/ics/asterix/runtime/evaluators/functions/records/RecordAddFieldsDescriptor.java
File asterix-runtime/src/main/java/edu/uci/ics/asterix/runtime/evaluators/functions/records/RecordAddFieldsDescriptor.java:

Line 187:                             IVisitablePointable valuePointable = null;
> Can these variable be moved up and reused?
Done


Line 192:                                 IVisitablePointable fieldName = names.get(j);
> Can this variable be moved up and resulted?
Done


https://asterix-gerrit.ics.uci.edu/#/c/298/7/asterix-runtime/src/main/java/edu/uci/ics/asterix/runtime/evaluators/visitors/RecordDeepEqualityAccessor.java
File asterix-runtime/src/main/java/edu/uci/ics/asterix/runtime/evaluators/visitors/RecordDeepEqualityAccessor.java:

Line 39:     private final int TABLE_SIZE = 100;
> In our meeting on Friday they talked about a way to do this dynamically. Pl
I added a class instantiation to make setting the table size value to another value different from the default 100. However, I would suggest revisiting "BinaryHashMap" (design change) to allow a more dynamic changes of the size instead. Note that this would also induce object generation rather than using an array of primitive objects. Thus I would suggest postponing this to a next version. Currently "SimilarityJoin" is using BinaryHashMap, which should also be updated, as well.


Line 40:     private final int TABLE_FRAME_SIZE = 32768;
> Can you use the Hyracks variable for frame size?
Done


Line 104:             keyEntry.set(buf, off, len);
> In several places you make buf, off, len for the set function. Is there a r
I did this for readability. In any cases, I have no other objections against just passing the directly.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3621ebdd71c7cd91b50d77a972ad863cea7fcbc2
Gerrit-PatchSet: 7
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Heri Ramampiaro <he...@gmail.com>
Gerrit-Reviewer: Heri Ramampiaro <he...@gmail.com>
Gerrit-Reviewer: Ian Maxon <im...@apache.org>
Gerrit-Reviewer: Jenkins <je...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Preston Carman <pr...@apache.org>
Gerrit-Reviewer: Steven Jacobs <sj...@ucr.edu>
Gerrit-HasComments: Yes