Ali Alsuliman has posted comments on this change. Change subject: [NO ISSUE][FUN] Implement array functions p3 ......................................................................
Patch Set 3: (23 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: Input lists to the array function are of different types > "Input contains different types"? Or would that bee to vague? I changed it to "Input contains different list types" 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 Sure. created to make it clearer. PUT would've done the same job. Line 1523: addFunction(ARRAY_FLATTEN, AListTypeComputer.INSTANCE_PUT, true); > why do we use PUT's type computer for FLATTEN? Their signatures are differe I created a new instance now in AListFirstTypeComputer for FLATTEN to make it clearer. PUT would do the same job even when the signatures are different number-wise and type-wise (that's only the case because we don't use checkArgType()). 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 l I actually changed the error message and removed "at least" so that "min args" and "max args" will be in its place. But I will make it now as you said. 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 Done 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. Alright. I took a note to file an issue. 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 Done 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 arg Done 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-ope Done Line 143: if (Math.floor(depth) < depth) { > How NaN, +-INF are handled here? Include here and return null? Line 187: ListAccessor newListAccessor = new ListAccessor(); > pool instances Done https://asterix-gerrit.ics.uci.edu/#/c/2790/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayIntersectDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayIntersectDescriptor.java: PS2, Line 148: listsEval[i] > pull out? (Also avoids repeated array access with bounds checking.) Done. You were referring to the listsArgs, right? 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 typ Done Line 146: if (!listType.isListType() || Math.floor(maxDouble) < maxDouble || targetTag == ATypeTag.NULL) { > how do we handle NaN, +-INF here? Same as array_flatten()? 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: PS2, Line 34: array_union > arra_symdiff - fix docs Copy-paste error. I will remove the doc for now. There are other functions missing the docs as well. Will do them together later. Line 57: } > override type inferer and make types fully-open Done 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: PS2, Line 37: pre > Keep the docs always on the descriptor? Sure PS2, Line 38: array_union > array_symdiff - fix docs Removed for now. Will put it later with the other functions missing the docs. Line 122: sameHashes.add(new ValueCounter(item, listIndex, 1)); > can we pool and reuse ValueCounter objects? Done https://asterix-gerrit.ics.uci.edu/#/c/2790/2/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArraySymDiffnDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArraySymDiffnDescriptor.java: PS2, Line 34: array_union > array_symdiffn - fix docs Same here 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: PS2, Line 46: where one null is for the missing item : * and the second null for the null item. > that doesn't seem to make sense ... but I guess that's the way it is. I agree. There are other functions that do the same thing, I believe. Line 67: } > override type inferer and produce fully open types Done 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? Done -- 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: 3 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: Till Westmann <[email protected]> Gerrit-HasComments: Yes
