Till Westmann has posted comments on this change. Change subject: [ASTERIXDB-2516][RT] add support for array deep comparison ......................................................................
Patch Set 2: (7 comments) https://asterix-gerrit.ics.uci.edu/#/c/3173/1/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/common/ListAccessorUtil.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/common/ListAccessorUtil.java: PS1, Line 53: getOrWriteItem Just call this getItem? The writing just seems to be an internal mechanism to enable the getting. https://asterix-gerrit.ics.uci.edu/#/c/3173/2/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/common/ListAccessorUtil.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/common/ListAccessorUtil.java: PS2, Line 53: getOrWriteItem Just call this "getItem"? I think that the writing is just an implementation that enables the getting. The return type would still report the difference in behavior. PS2, Line 64: ATypeTag itemTag = listItemTag; : boolean itemIncludesTag = listItemTag == ATypeTag.ANY; : if (itemIncludesTag) { : itemTag = EnumDeserializer.ATYPETAGDESERIALIZER.deserialize(listBytes[itemOffset]); : } I think that boolean itemIncludesTag = listItemTag == ATypeTag.ANY; final ATypeTag itemTag = itemIncludesTag ? EnumDeserializer.ATYPETAGDESERIALIZER.deserialize(listBytes[itemOffset]) : listItemTag; is easier to read (but I might be wrong :) ). https://asterix-gerrit.ics.uci.edu/#/c/3173/2/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/LogicalComplexBinaryComparator.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/LogicalComplexBinaryComparator.java: PS2, Line 67: comparisonResult It seems that we're adding another option to the Result enum (one that we could not switch on). Should we make this explicit and add another value? PS2, Line 101: if (result == Result.LT) { : return Result.GT; : } else if (result == Result.GT) { : return Result.LT; : } : return result; maybe switch (result) { case LT: return Result.GT; case GT: return Result.LT; default: return result; } ? PS2, Line 122: byte[] leftBytes, int leftStart, : int leftLen Should we pass a Pointable here? https://asterix-gerrit.ics.uci.edu/#/c/3173/2/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/util/container/IObjectPool.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/util/container/IObjectPool.java: PS2, Line 41: markFree Just call it "free"? -- To view, visit https://asterix-gerrit.ics.uci.edu/3173 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2fef48d7c6189362f44786b8d89d89c5f91d4b10 Gerrit-PatchSet: 2 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
