>From Hussain Towaileb <[email protected]>: Hussain Towaileb 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 18: Code-Review+1 (8 comments) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/17/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractConcatStringEval.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractConcatStringEval.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/17/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractConcatStringEval.java@149 PS17, Line 149: typeTag == ATypeTag.NULL > minor, I think typeTag is never NULL when we reach to this code You're right, done. Deep check would've checked everything and finished, basic check wouldn't have had the chance to set it to NULL yet. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/17/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractConcatStringEval.java@289 PS17, Line 289: UTF8StringUtil.getUTFLength(separatorBytes, separatorStartOffset + 1) > We can move this out of the for-loop. That is calculate it only once and then > use it here. […] You're right, done. It's a fixed length, so calculating it once is enough. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/17/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractConcatStringEval.java@304 PS17, Line 304: tempLengthArray > we should reset tempLengthArray and fill it with 0s before Done 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 Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/13/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/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/PointableHelper.java@322 PS13, Line 322: if (listAccessor.getItemType() == ATypeTag.MISSING) { > I'd move this into the else block of the next if statement: if (listAccessor. > […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/4023/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/PointableHelper.java@336 PS13, Line 336: if (typeTag == ATypeTag.NULL) { > what about NULLs in the list? Seems inconsistent that they are not handled by > this method. Done 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. […] Makes sense, done. 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". That makes sense, done. -- 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: 18 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-CC: Anon. E. Moose #1000209 Gerrit-Comment-Date: Mon, 09 Dec 2019 15:52:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Ali Alsuliman <[email protected]> Comment-In-Reply-To: Dmitry Lychagin <[email protected]> Gerrit-MessageType: comment
