>From Ali Alsuliman <[email protected]>: Ali Alsuliman has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023 )
Change subject: [ASTERIXDB-2667][FUN] Share code base between string functions ...................................................................... Patch Set 14: (6 comments) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/14/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ConcatStringEval.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ConcatStringEval.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/14/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ConcatStringEval.java@43 PS14, Line 43: function evaluator https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/14/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ConcatStringEval.java@148 PS14, Line 148: if (separatorTypeTag != ATypeTag.STRING) { : ExceptionUtil.warnTypeMismatch(ctx, sourceLoc, functionIdentifier, separatorTypeTag.serialize(), : separatorPosition, ATypeTag.STRING); : PointableHelper.setNull(result); : return; : } I think we also need to wait here for functions not doing isPerformDeepCheck. Could you try string_join(["str", missing], 5) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/14/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ConcatStringEval.java@164 PS14, Line 164: if (isAcceptListOnly) { : if (!typeTag.isListType()) { : PointableHelper.setNull(result); : ExceptionUtil.warnTypeMismatch(ctx, sourceLoc, functionIdentifier, typeTag.serialize(), i, : expectedTypeList); : return; : } : } If we are making this evaluator generic, then I think we need to wait until we make sure there is no missing in the list in the case of "no checking elements first". Probably having one flag to determine the current two different behaviour would make the logic easier instead of having isPerformDeepCheck and isAcceptListOnly to make the evaluator generic. i.e. 1. accept only lists and no deep checking elements. 2. accept lists and strings and do deep checking elements. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/14/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/PointableHelper.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/PointableHelper.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/14/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/PointableHelper.java@328 PS14, Line 328: if (listAccessor.itemsAreSelfDescribing()) { : for (int i = 0; i < listAccessor.size(); i++) { : if (listAccessor.getItemType(listAccessor.getItemOffset(i)) == ATypeTag.MISSING) { : return PointableValueState.MISSING; : } : : if (listAccessor.getItemType(listAccessor.getItemOffset(i)) == ATypeTag.NULL) { : isNullMet = true; : } : } : } : // List item type is MISSING : else if (listAccessor.getItemType() == ATypeTag.MISSING) { : return PointableValueState.MISSING; : } : // List item type is NULL : else if (listAccessor.getItemType() == ATypeTag.NULL) { : return PointableValueState.NULL; : } I think it's simpler to just have the for-loop. Theoretically, you can have an empty list whose item type is MISSING or NULL in which case the current logic would return MISSING/NULL even though the list does not have any element. I know it's hard to get the system in that state but the API semantics says that it will search for MISSING/NULL elements inside the list. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/14/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/StringJoinDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/StringJoinDescriptor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/14/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/StringJoinDescriptor.java@48 PS14, Line 48: StringJoinDescriptor The type computer needs to be fixed for string_join(). We should make it return AUnionType.createUnknownableType on invalid args (similar to ConcatTypeComputer. They could be combined actually if you feel you want to do that). https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/14/hyracks-fullstack/hyracks/hyracks-util/src/main/java/org/apache/hyracks/util/string/UTF8StringUtil.java File hyracks-fullstack/hyracks/hyracks-util/src/main/java/org/apache/hyracks/util/string/UTF8StringUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/14/hyracks-fullstack/hyracks/hyracks-util/src/main/java/org/apache/hyracks/util/string/UTF8StringUtil.java@152 PS14, Line 152: not the type position maybe remove this part? we are in Hyracks. Hyracks does not have the notion of "type". -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: I41b644c6841b222d1c6c529b2f9189f42178e28c Gerrit-Change-Number: 4023 Gerrit-PatchSet: 14 Gerrit-Owner: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Comment-Date: Wed, 27 Nov 2019 07:17:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
