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 <[email protected]> Gerrit-Reviewer: Heri Ramampiaro <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Preston Carman <[email protected]> Gerrit-Reviewer: Steven Jacobs <[email protected]> Gerrit-HasComments: Yes
