Hussain Towaileb 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: (14 comments) https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/BitwiseValueCountFlagTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/BitwiseValueCountFlagTypeComputer.java: https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/BitwiseValueCountFlagTypeComputer.java@45 PS11, Line 45: > this class and BitwiseValuePositionFlagTypeComputer and BitwiseValuePositio Done https://asterix-gerrit.ics.uci.edu/#/c/3377/11/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/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitMultipleValuesEvaluator.java@94 PS11, Line 94: if (result.get > resultStorage is already reset down, right? You're right, done. https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitMultipleValuesEvaluator.java@102 PS11, Line 102: > better to use getStartOffset Done https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitMultipleValuesEvaluator.java@122 PS11, Line 122: } catch (TypeMismatchException ex) { : throw new TypeMismatchException(sourceLoc, functionIdentifier, 0, typeTag.serialize(), : ATypeTag.SERIALIZED_INT8_TYPE_TAG, ATypeTag.SERIALIZED_INT16_TYPE_TAG, : ATypeTag.SERIALIZED_INT32_TYPE_TAG, ATypeTag.SERIALIZED_INT64_TYPE_TAG, : ATypeTag.SERIALIZED_FLOAT_TYPE_TAG, ATypeTag.SERIALIZED_DOUBLE_TYPE_TAG); : } : : // Loop and do the bitwise operation for all the arguments (start from index 1, we did first argument above) : for (int i = 1; i < argPointables.length; i++) { : > we could refactor this code and reuse below (and for other bit functions, t Done https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitMultipleValuesEvaluator.java@154 PS11, Line 154: > you could plug it directly in the method if you want. Refactored to a static method in utils. https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitMultipleValuesEvaluator.java@185 PS11, Line 185: > The returned result is always inverted. we could call it "isInvalidLongValu True, I noticed the warning, but it felt more readable to check for validity rather than invalidity of something, what do you think? https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitMultipleValuesEvaluator.java@189 PS11, Line 189: > what is the behaviour if the float is NaN or INF/-INF? They're excluded by the limits check, > MAX_VALUE and < MIN_VALUE. I added test cases for INF/-INF. https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitMultipleValuesEvaluator.java@192 PS11, Line 192: > what if the value is < MIN_VALUE? do these functions accept negative argume Done https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitMultipleValuesEvaluator.java@219 PS11, Line 219: > we should refactor this and reuse in other bit functions. you could put it Done https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitValueCountFlagEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitValueCountFlagEvaluator.java: https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitValueCountFlagEvaluator.java@174 PS11, Line 174: > are we losing precision here? You're right, we were losing precision, that has been fixed. https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitValuePositionEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitValuePositionEvaluator.java: https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitValuePositionEvaluator.java@53 PS11, Line 53: > we should probably mention whether the position is counted from left or rig Done https://asterix-gerrit.ics.uci.edu/#/c/3377/11/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/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/AbstractBitValuePositionFlagEvaluator.java@83 PS11, Line 83: al IPointable valuePointable = new Vo > this class and AbstractBitValuePositionEvaluator are quite similar. can we Done https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/BitShiftWithRotateFlagDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/BitShiftWithRotateFlagDescriptor.java: https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/BitShiftWithRotateFlagDescriptor.java@54 PS11, Line 54: > I would just pass a flag and make the evaluator not abstract. Inheritance l Done https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/BitTestWithAllFlagDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/BitTestWithAllFlagDescriptor.java: https://asterix-gerrit.ics.uci.edu/#/c/3377/11/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/BitTestWithAllFlagDescriptor.java@54 PS11, Line 54: torFactory createEvaluatorFactory(final IScalarEvaluatorFactory > i would pass a flag instead. Done -- 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: Fri, 17 May 2019 16:50:41 +0000 Gerrit-HasComments: Yes
