Ali Alsuliman has posted comments on this change. Change subject: [NO ISSUE][FUN] Implement array functions 1 ......................................................................
Patch Set 5: (13 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 Removed now. This should've been removed. The first array functions I added were code-gen and they require type settings. But now there is another issue with this test case. It does not check for HYR exceptions thrown by functions. I changed the test case since now some of my array functions do throw HYR exceptions. Please, check (not really sure about the error code range though) 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 This should've been removed. 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? Yes. I'll change the name and add comments. 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) That's right. TypeComputerUtils.getResultType() does the same thing for nullable case. Here I want to return a list that is nullable and missable. 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: Line 83: * @return -1 if position value is missing, -2 if null, otherwise should return the adjusted position value, >= 0 > minor. let's create constants for -1 and -2. (RETURN_NULL, RETURN_MISSING) Done Line 110: CastTypeEvaluator caster = new CastTypeEvaluator(); > create CastTypeEvaluator in the constructor and reuse instance Done Line 147: listBuilder = new OrderedListBuilder(); > we need to reuse list builder instances. let's create 2 fields: listBuilder But then I'll have to move the code just below the if-else and put it twice. Want to do that? Line 163: ListAccessor listAccessor = new ListAccessor(); > create ListAccessor in the constructor. Done PS5, Line 184: for (IPointable addedValues : values) { > Let's remove all the for each in the execution critical path? Done 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: Line 87: ATypeTag listTag = > it's a bit confusing that we might get AListPointable here that is not a li The issue is that I was trying to cast VoidPointable to AListPointable which does not work. But anyways, I changed it now and we don't need AListPointable. All I needed was the number of items in the serialized list which I can get from AOrderedListSerializerDeserializer & AUnorderedListSerializerDeserializer. Please, check. PS5, Line 92: 2 > -2 means something? replace by a static final member with a name? Done 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? Done 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 Done -- 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: 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
