Dmitry Lychagin has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/3377 )
Change subject: [ASTERIXDB-2562][FUN] Add support for bitwise functions ...................................................................... Patch Set 13: (12 comments) Not done yet, but here's the first batch of comments. https://asterix-gerrit.ics.uci.edu/#/c/3377/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/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/PointableHelper.java@317 PS13, Line 317: if (value > Long.MAX_VALUE || value < Long.MIN_VALUE || value > Math.floor(value)) { Do we need to handle NaN here? https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/PointableHelper.java@327 PS13, Line 327: if (value > Long.MAX_VALUE || value < Long.MIN_VALUE || value > Math.floor(value)) { Do we need to handle NaN here? https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/PointableHelper.java@351 PS13, Line 351: public static long getLongValue(byte[] bytes, int startOffset, ATypeTag typeTag) throws HyracksDataException { We already have ATypeHierarchy.getLongValue(). Can we use that instead of creating a new method? https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitMultipleValuesEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitMultipleValuesEvaluator.java: https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitMultipleValuesEvaluator.java@150 PS13, Line 150: applyBitwiseOperation(resultMutableInt64.getLongValue(), nextValue); there's no need to use resultMutableInt64 at all in this loop. If you change applyBitWiseOperation() to return 'long' instead of 'void', then we can avoid writing/reading to/from resultMutableInt64 while looping. We'll only need to write the value there to serialize the result https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitSingleValueEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitSingleValueEvaluator.java: https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitSingleValueEvaluator.java@58 PS13, Line 58: final AMutableInt32 resultMutableInt32 = new AMutableInt32(0); bitnot() only uses int64 mutable/serde, but bitcount() only uses int32 mutable/serde, but this abstract evaluator always creates both pairs. Should we create move those mutable/serdes to each subclass, so only one pair is created? https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitSingleValueEvaluator.java@103 PS13, Line 103: resultMutableInt64.setValue(PointableHelper.getLongValue(bytes, startOffset + 1, typeTag)); why do we need to write the value to resultMutableInt64 and then immediately read it from there in line 111? just pass the long value we got from getLongValue() to applyBitwiseOperation() https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitValuePositionFlagEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitValuePositionFlagEvaluator.java: https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitValuePositionFlagEvaluator.java@85 PS13, Line 85: final IPointable flagPointable = new VoidPointable(); We don't need to create this object if there's no flagEvaluator available. https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitValuePositionFlagEvaluator.java@104 PS13, Line 104: flagEvaluator = hasFlagArgument ? argEvaluatorFactories[2].createScalarEvaluator(context) : null; do you need hasFlagArgument at all? Check the length of argEvaluatorFactories and if it's more than 2 then we have it, otherwise not. https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitValuePositionFlagEvaluator.java@123 PS13, Line 123: if (PointableHelper.checkAndSetMissingOrNull(result, valuePointable, positionPointable, flagPointable)) { if 'flagEvaluator' is null then flagPointable is not properly initialized. Will we get NPE when checkAndSetMissingOrNull() tries to access it? Do we have tests for this if not, let's add them. I think it this case we should use checkAndSetMissingOrNull() that takes 2 pointable arguments instead of 3. https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitValuePositionFlagEvaluator.java@161 PS13, Line 161: return; do we ever return anything other than NULL if applyBitwiseOperation() returns false? If it can only be NULL then let's set it here using PointableHelper.setNull() instead of doing it in every implementation method. https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/BitClearDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/BitClearDescriptor.java: https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/BitClearDescriptor.java@128 PS13, Line 128: for (int i = 0; i < listAccessor.size(); i++) { would it make sense to move list iteration into the parent class so it's not repeated by all subclasses? https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/BitClearDescriptor.java@130 PS13, Line 130: listAccessor.getOrWriteItem(i, listItemPointable, resultStorage); you need to check the returned value of getOrWriteItem() because the item can either be set into provided pointable or written into provided storage. only the returned boolean can tell you what happened. read getOrWriteItem() javadoc for more. -- To view, visit https://asterix-gerrit.ics.uci.edu/3377 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I70a6376d6ca12da55eeff88fa0b1c85f970ef8e6 Gerrit-Change-Number: 3377 Gerrit-PatchSet: 13 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: Hussain Towaileb <hussai...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Comment-Date: Tue, 21 May 2019 00:13:14 +0000 Gerrit-HasComments: Yes
