Ali Alsuliman has posted comments on this change. Change subject: [ASTERIXDB-2516][RT] add support for array deep comparison ......................................................................
Patch Set 7: (18 comments) https://asterix-gerrit.ics.uci.edu/#/c/3173/6/asterixdb/asterix-app/src/test/resources/runtimets/results/comparison/arrays/arrays.004.regexjson File asterixdb/asterix-app/src/test/resources/runtimets/results/comparison/arrays/arrays.004.regexjson: PS6, Line 1: { > Could this be an adm file? I will change all .regexjson to .adm later https://asterix-gerrit.ics.uci.edu/#/c/3173/6/asterixdb/asterix-app/src/test/resources/runtimets/results/comparison/arrays/arrays.020.regexjson File asterixdb/asterix-app/src/test/resources/runtimets/results/comparison/arrays/arrays.020.regexjson: PS6, Line 3: false > This is because there's no '-0' in the integer domain? I would assume so. PS6, Line 10: NaN > Not sure how 'NaN' should compare. Is this result consistent with the Java Not sure either. We can look at it separately. https://asterix-gerrit.ics.uci.edu/#/c/3173/6/asterixdb/asterix-app/src/test/resources/runtimets/results/comparison/arrays/arrays.021.regexjson File asterixdb/asterix-app/src/test/resources/runtimets/results/comparison/arrays/arrays.021.regexjson: PS6, Line 8: "[1,2] < [1,2,missing]", "r": true > This is interesting. It seems that the length comparison kicks in before lo Yes, length comparison kicks in when all pairs evaluate to equal (plus other conditions). Not sure about how others do it. PS6, Line 13: "[null,1] = [1,1,3]", "r": false > Should this be 'null'? I guess we always know the answer no matter what null value is. PS6, Line 18: false > null? 99 != 1. So, no matter what null is, we will end up with a false answer. PS6, Line 24: false > null? 3 != 99 regardless of the null value. PS6, Line 25: false > missing? The answer is always known due to the fact that 3 != 99 PS6, Line 26: false > missing? 4 != 99 PS6, Line 27: false > missing? We've got 5 != 99 which should make the statement false. PS6, Line 28: false > missing? We've got 5 != 9 which should make the statement false. PS6, Line 32: true > null? The statement is always true because 3 != 99 is always true regardless of the null value. https://asterix-gerrit.ics.uci.edu/#/c/3173/6/asterixdb/asterix-app/src/test/resources/runtimets/results/comparison/arrays/arrays.022.regexjson File asterixdb/asterix-app/src/test/resources/runtimets/results/comparison/arrays/arrays.022.regexjson: PS6, Line 6: false > null? Same idea here. If we know the answer for sure, null value does not make a difference. PS6, Line 8: false > null? Same here. PS6, Line 9: false > null? Same here. PS6, Line 10: "t9" > stopping here. It's null here because 'string' is incomparable to 2, not because of the null. https://asterix-gerrit.ics.uci.edu/#/c/3173/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/LogicalScalarBinaryComparator.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/LogicalScalarBinaryComparator.java: Line 104: throw new IllegalStateException("Two different non-numeric tags but they are compatible"); > The message should say " ... NOT compatible", right? I think I meant compatible. I check at line 87 if the two types are compatible and then proceed. So, starting line 95, the two types are compatible for sure. They could be (1) numbers (2) non-numbers. The exception here is for (2) since I don't think we have compatible tags which are not number yet, like DATE and DATETIME. https://asterix-gerrit.ics.uci.edu/#/c/3173/7/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: Line 41: boolean free(E element); > minor. add javadoc. Sure. I'll add it in the following change. -- 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: 7 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
