Ali Alsuliman 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 12: (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: BitwiseValueCountFlagTypeComputer this class and BitwiseValuePositionFlagTypeComputer and BitwiseValuePositionTypeComputer have a lot in common. you could either create instances with several flags (return type, whether to include ARRAY), or create instances with an enum that differentiates each one. Based on the enum (or flags) you check and configure the return type. 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: resultStorage.reset(); resultStorage is already reset down, right? 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: 0 better to use getStartOffset 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: if (!ATypeHierarchy.canPromote(typeTag, ATypeTag.DOUBLE)) { : PointableHelper.setNull(result); : return; : } : : // Value validity check : if (!isValidLongValue(bytes, startOffset + 1, typeTag)) { : PointableHelper.setNull(result); : return; : } we could refactor this code and reuse below (and for other bit functions, too). The refactored method would return boolean (true to return, false to continue). It would also set the result if it says true. 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: getLongValue(bytes, startOffset + 1, typeTag, i) you could plug it directly in the method if you want. 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: isValidLongValue The returned result is always inverted. we could call it "isInvalidLongValue" and change accordingly. 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: float value what is the behaviour if the float is NaN or 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: value > Long.MAX_VALUE what if the value is < MIN_VALUE? do these functions accept negative arguments? 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: private long getLongValue(byte[] bytes, int startOffset, ATypeTag typeTag, int argIndex) we should refactor this and reuse in other bit functions. you could put it in a util class. 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: (int) count) are we losing precision here? 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: * This function receives two arguments we should probably mention whether the position is counted from left or right. 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: AbstractBitValuePositionFlagEvaluator this class and AbstractBitValuePositionEvaluator are quite similar. can we combine and extend somehow? 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: argEvaluatorFactories[2].createScalarEvaluator(context); I would just pass a flag and make the evaluator not abstract. Inheritance looks simple. 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: return argEvaluatorFactories[2].createScalarEvaluator(context); i would pass a flag instead. -- 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: 12 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: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Comment-Date: Wed, 15 May 2019 08:03:06 +0000 Gerrit-HasComments: Yes
