abdullah alamoudi has posted comments on this change. Change subject: [NO ISSUE][FUN] Implement array functions 1 ......................................................................
Patch Set 5: (8 comments) Looks good overall.. small comments. https://asterix-gerrit.ics.uci.edu/#/c/2751/5/asterixdb/asterix-app/src/test/java/org/apache/asterix/runtime/ExceptionIT.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/runtime/ExceptionIT.java: PS5, Line 55: "Array") : && !splits[splits.length - 1].startsWith("AbstractArray" why are we excluding array functions https://asterix-gerrit.ics.uci.edu/#/c/2751/5/asterixdb/asterix-app/src/test/java/org/apache/asterix/runtime/NullMissingTest.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/runtime/NullMissingTest.java: PS5, Line 44: private static final String arrayName = "Array"; : private static final String abstractArray = "AbstractArray"; make public in one place and reference from ExceptionIT? also static final -> all caps? https://asterix-gerrit.ics.uci.edu/#/c/2751/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/functions/BuiltinFunctions.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/functions/BuiltinFunctions.java: PS5, Line 1481: AListOrNullTypeComputer does this also imply or missing? https://asterix-gerrit.ics.uci.edu/#/c/2751/5/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/AListOrNullTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/AListOrNullTypeComputer.java: PS5, Line 70: createUnknownableType I think this also includes missing. (Y) https://asterix-gerrit.ics.uci.edu/#/c/2751/5/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractArrayAddRemoveEval.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractArrayAddRemoveEval.java: PS5, Line 184: for (IPointable addedValues : values) { Let's remove all the for each in the execution critical path? As each call will have to create an iterator https://asterix-gerrit.ics.uci.edu/#/c/2751/5/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayAppendDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayAppendDescriptor.java: PS5, Line 92: 2 -2 means something? replace by a static final member with a name? https://asterix-gerrit.ics.uci.edu/#/c/2751/5/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayInsertDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayInsertDescriptor.java: PS5, Line 97: -1 replace those values with a member with a meaningful name? https://asterix-gerrit.ics.uci.edu/#/c/2751/5/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayRemoveDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayRemoveDescriptor.java: PS5, Line 109: for (IPointable removedVal : removedVals) { same here -- To view, visit https://asterix-gerrit.ics.uci.edu/2751 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7d9cb80325138daf99fb039793446d109481c94b Gerrit-PatchSet: 5 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
