Hussain Towaileb has posted comments on this change. Change subject: [WIP] Code generator alternative ......................................................................
Patch Set 8: (4 comments) Note: Please read the comment response in the PointableHelper class first about the varargs, as all the other comments are related to it. As for the Tests and plugin, I'm looking into that right now to see what needs to be done, thanks! https://asterix-gerrit.ics.uci.edu/#/c/3043/8/asterixdb/asterix-fuzzyjoin/src/main/java/org/apache/asterix/runtime/evaluators/functions/EditDistanceStringIsFilterableEvaluator.java File asterixdb/asterix-fuzzyjoin/src/main/java/org/apache/asterix/runtime/evaluators/functions/EditDistanceStringIsFilterableEvaluator.java: PS8, Line 86: PointableHelper.checkMissingOrNull(result, stringPtr, isReturnNull) : || PointableHelper.checkMissingOrNull(result, edThreshPtr, isReturnNull) : || PointableHelper.checkMissingOrNull(result, gramLenPtr, isReturnNull) : || PointableHelper.checkMissingOrNull(result, usePrePostPtr, isReturnNull) > see comment in PointableHelper, if checkMissingOrNull used vararg, we can m Please check the comment I put in the PointableHelper related to this part. PS8, Line 93: // Null argument encountered : if (isReturnNull.getValue()) { : PointableHelper.setNull(result); : return; : } > it seems like we usually (always?) do this in the case of null-- why not se Indeed you're right, but there are 2 cases where the arguments are not straight forward: - Arguments are a list that needs to be looped. (Example: AbstractFindBinaryEvaluator and ArrayIntersectEval) - Couple of arguments, then a list of arguments. (Example: FullTextContainsEvaluator) As missing has a higher priority than null, I gotta go over all the arguments and confirm they're not missing, if that's the case, then I check the null, and hence, the reason I'm using the MutableBoolean keeping an eye on the null flag. But I agree with you, there might be a better way for this, probably creating another method, however, this is related to the original problem (check the other comment about varargs), would it be ok to pass varargs as parameters, so the PointableHelper can set the final result in one go. https://asterix-gerrit.ics.uci.edu/#/c/3043/8/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: PS8, Line 161: IPointable argument > make a vararg, to enable checking multiple args in single invocation I originally had it as passing varargs in the previous patch set, however, wouldn't this end up creating a new array internally (for the varargs) each time this method is invoked? And since this is invoked per tuple, wouldn't that end up causing a performance overhead? PS8, Line 171: // MISSING check : if (data[offset] == ATypeTag.SERIALIZED_MISSING_TYPE_TAG) { : setMissing(result); : return true; : } : : // NULL check (Only flag at this stage) : if (data[offset] == ATypeTag.SERIALIZED_NULL_TYPE_TAG) { : setNull(result); : isNull.setValue(true); : } : : // This is reached only if it's not MISSING or NULL : return false; > consider using an enum to reflect the three states (NULL, MISSING, PRESENT( That's what I'm currently contemplating. Although it makes more sense to return it that way (enum way), the usefulness of that depends on whether the use of varargs is viable or not. Take this as an example: (in case varargs is not viable) Say we have 3 arguments, the checks will have to go this way (simplified): - I need to check all the missing first. - I need to check all the null next. If (arg1Result == MISSING || arg2Result == MISSING || arg3Result == MISSINg) { return; } If (arg1Result == NULL || arg2Result == NULL || arg3Result == NULl) { PointableHelper.setNull(result); // probably can remove this return; } This felt more complicated and resource consuming than just passing the MutableBoolean and letting it hold the reference to the null result flag. The culprit behind all of this is still the fact whether varargs is viable or not. -- To view, visit https://asterix-gerrit.ics.uci.edu/3043 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icca2e2128c4b0f2bfd8675655cf5296cbbaeba88 Gerrit-PatchSet: 8 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Hussain Towaileb <hussai...@gmail.com> Gerrit-Reviewer: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <dmitry.lycha...@couchbase.com> Gerrit-Reviewer: Hussain Towaileb <hussai...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Michael Blow <mb...@apache.org> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-HasComments: Yes
