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 <[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
