asterixdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Preston Carman (Code Review)" <do-not-re...@asterix-gerrit.ics.uci.edu>
Subject Change in asterixdb[master]: Clean-up - Made record-merge visible from AQL + more proper ...
Date Mon, 20 Jul 2015 21:20:20 GMT
Preston Carman 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)

Here are a few of my thoughts.

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?


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


Line 2309:           "address":{"state":"CA"}, 
Does the function change the order of fields? If so, note that otherwise update the result.


Line 2548:  * Arguments:
Do think we should make some notes here about what equality means? Based on our friday meeting,
I wonder if we need to add some detail to this explanation.


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.


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?


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?


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.


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?


Line 84:                             Pair<IVisitablePointable, Boolean>(accessor1, Boolean.FALSE);
Why Boolean over boolean?


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? This would allow
for object allocation to be defined by calling functions. Allowing reusing of the UTF8StringPointable.


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?


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


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. Please look into
this.


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


Line 104:             keyEntry.set(buf, off, len);
In several places you make buf, off, len for the set function. Is there a reason to make them
variables as opposed to just passing them as arguments?


-- 
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 <heriram@gmail.com>
Gerrit-Reviewer: Heri Ramampiaro <heriram@gmail.com>
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Preston Carman <prestonc@apache.org>
Gerrit-Reviewer: Steven Jacobs <sjaco002@ucr.edu>
Gerrit-HasComments: Yes

Mime
View raw message