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 14: (14 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: > Do we need to handle NaN here? Done. 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: > Do we need to handle NaN here? Done. 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: > We already have ATypeHierarchy.getLongValue(). Can we use that instead of c Done. I was not aware of such a method, I removed the new method and switched all to use ATypeHierarchy.getLongValue(). 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: > there's no need to use resultMutableInt64 at all in this loop. Done 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: FunctionIdentifier functionIdentifier, SourceLocation sourceLocation) throws HyracksDataException { > bitnot() only uses int64 mutable/serde, but bitcount() only uses int32 muta Done 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: > why do we need to write the value to resultMutableInt64 and then immediatel Done 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: > We don't need to create this object if there's no flagEvaluator available. Done 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: > do you need hasFlagArgument at all? Check the length of argEvaluatorFactori Done 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 'flagEvaluator' is null then flagPointable is not properly initialized. checkAndSetMissingOrNull() only assumes the first 2 arguments are not null, first argument is the result Pointable to write to, second argument is the first pointable to be checked. So passing null for the other pointables will have no impact and cause no issues. Although I'm wondering if I should check if the result pointable and first pointable are null before using them, however, ideally, I thought it might be safe to assume that when this method is called, at least the result to write to and the first pointable should be not null, because if those 2 are null, then something is off and an exception needs to be thrown. What do you think? 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: > do we ever return anything other than NULL if applyBitwiseOperation() retur Done 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: > would it make sense to move list iteration into the parent class so it's no I refactored the 2 classes to avoid repetitive code. This originally originated from trying to share the classes BitValuePositionEvaluator and BitValuePositionFlagEvaluator since they have quite a few in common. But that can get messy, so I split the 2 classes to keep the code much cleaner. 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: > you need to check the returned value of getOrWriteItem() because the item c It seems in both cases, the item gets written eventually to the provided pointable, no? https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/IsBitSetWithoutAllFlagDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/IsBitSetWithoutAllFlagDescriptor.java: https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/bitwise/IsBitSetWithoutAllFlagDescriptor.java@82 PS13, Line 82: > is flagEvaluator always 'null' here? This is 'WithoutAllFlag' function. Is I refactored the 2 classes to avoid repetitive code. This originally originated from trying to share the classes BitValuePositionEvaluator and BitValuePositionFlagEvaluator since they have quite a few in common. But that can get messy, so I split the 2 classes to keep the code much cleaner. https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/exceptions/TypeMismatchException.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/exceptions/TypeMismatchException.java: https://asterix-gerrit.ics.uci.edu/#/c/3377/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/exceptions/TypeMismatchException.java@74 PS13, Line 74: public TypeMismatchException(SourceLocation sourceLoc, byte actualTypeTag, byte... expectedTypeTags) { > This new constructor is only used by the newly added PointableHelper.getLon You're right, 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: 14 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: Wed, 22 May 2019 16:15:11 +0000 Gerrit-HasComments: Yes
