Dmitry Lychagin has posted comments on this change. Change subject: [NO ISSUE][FUN] Implement array functions p3 ......................................................................
Patch Set 2: (17 comments) https://asterix-gerrit.ics.uci.edu/#/c/2790/2/asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties File asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties: PS2, Line 75: 38 > where is 37? 37 is above, in line 41: (37,1091) https://asterix-gerrit.ics.uci.edu/#/c/2790/2/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: Line 1518: addFunction(ARRAY_UNION, AListTypeComputer.INSTANCE_PUT, true); Should we create a separate type computer for UNION/INTERSECT/etc functions? They expect all arguments to be arrays, unlike PUT. Line 1523: addFunction(ARRAY_FLATTEN, AListTypeComputer.INSTANCE_PUT, true); why do we use PUT's type computer for FLATTEN? Their signatures are different. FLATTEN has at most two arguments and 2nd argument is numeric, PUT has arbitrary number of arguments. https://asterix-gerrit.ics.uci.edu/#/c/2790/2/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/AListTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/AListTypeComputer.java: Line 67: throw new CompilationException(ErrorCode.COMPILATION_INVALID_NUM_OF_ARGS, expr.getSourceLocation(), numArgs, I think the error message might be confusing because the message says "at least" and then will insert "min args" and potentially "max args" there. I'd keep the message simple: just say "invalid number of arguments for function <NAME>". User can then find in the documentation the supported number of arguments for each function. https://asterix-gerrit.ics.uci.edu/#/c/2790/2/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/ArrayIfNullTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/ArrayIfNullTypeComputer.java: Line 38: return ((AbstractCollectionType) type).getItemType(); make return type nullable because the function returns null if the array is empty. https://asterix-gerrit.ics.uci.edu/#/c/2790/2/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/ArrayRangeTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/ArrayRangeTypeComputer.java: Line 34: public class ArrayRangeTypeComputer extends AbstractResultTypeComputer { we have a range() functions that implements a subset of this functionality. It only operates on integer. Let's make a follow up change that would add float/double support to that function and make array_range() an alias for range() https://asterix-gerrit.ics.uci.edu/#/c/2790/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractArrayProcessArraysEval.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractArrayProcessArraysEval.java: Line 180: protected class ValueCounter { This class is only used by ArraySymDiffEval, so let's move it there https://asterix-gerrit.ics.uci.edu/#/c/2790/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayConcatDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayConcatDescriptor.java: Line 53: public static final IFunctionDescriptorFactory FACTORY = new IFunctionDescriptorFactory() { How come we're not overriding createFunctionTypeInferer() here. We need argument types to convert argument values to be open, right? How come we're not doing this in AbstractArrayProcessArraysEval, like we did in AbstractArrayAddRemoveEval? https://asterix-gerrit.ics.uci.edu/#/c/2790/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayFlattenDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayFlattenDescriptor.java: Line 82: } how come we're not overriding type inferer here? We need return a fully-open list, right? Line 143: if (Math.floor(depth) < depth) { How NaN, +-INF are handled here? Line 187: ListAccessor newListAccessor = new ListAccessor(); pool instances https://asterix-gerrit.ics.uci.edu/#/c/2790/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayReplaceDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayReplaceDescriptor.java: Line 62: }; need to override type inferer and cast list and new value to fully open types, right? Line 146: if (!listType.isListType() || Math.floor(maxDouble) < maxDouble || targetTag == ATypeTag.NULL) { how do we handle NaN, +-INF here? https://asterix-gerrit.ics.uci.edu/#/c/2790/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArraySymDiffDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArraySymDiffDescriptor.java: Line 57: } override type inferer and make types fully-open https://asterix-gerrit.ics.uci.edu/#/c/2790/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArraySymDiffEval.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArraySymDiffEval.java: Line 122: sameHashes.add(new ValueCounter(item, listIndex, 1)); can we pool and reuse ValueCounter objects? https://asterix-gerrit.ics.uci.edu/#/c/2790/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayUnionDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayUnionDescriptor.java: Line 67: } override type inferer and produce fully open types Line 138: private boolean isNewItem(IPointable item, List<IPointable> sameHashes) throws HyracksDataException { this method is duplicated many times in this change. can we reuse it? -- To view, visit https://asterix-gerrit.ics.uci.edu/2790 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ida0d12d48f8c676d5a93b024c301dd13ef400247 Gerrit-PatchSet: 2 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: Till Westmann <[email protected]> Gerrit-HasComments: Yes
