Ali Alsuliman has posted comments on this change. Change subject: [NO ISSUE][FUN] Implement array functions p2 ......................................................................
Patch Set 12: (10 comments) https://asterix-gerrit.ics.uci.edu/#/c/2756/12/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/array_fun/array_distinct/array_distinct.4.query.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/array_fun/array_distinct/array_distinct.4.query.sqlpp: Line 20: use TinySocial; > can you add a comment to this and following query to say that the error is Done https://asterix-gerrit.ics.uci.edu/#/c/2756/12/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/array_fun/array_sort/array_sort.4.query.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/array_fun/array_sort/array_sort.4.query.sqlpp: Line 22: select array_sort([19, 3, [5]]); > add a comment to this and following query to say that the error is expected Done https://asterix-gerrit.ics.uci.edu/#/c/2756/12/asterixdb/asterix-app/src/test/resources/runtimets/results/array_fun/array_append/array_append.3.adm File asterixdb/asterix-app/src/test/resources/runtimets/results/array_fun/array_append/array_append.3.adm: Line 1: { "t1": [ { "$1": {{ "t-mobile", "customization", "sth", 5 }} }, { "$1": {{ "verizon", "voice-clarity", "sth", 5 }} }, { "$1": {{ "iphone", "platform", "sth", 5 }} }, { "$1": {{ "samsung", "voice-command", "sth", 5 }} }, { "$1": {{ "verizon", "shortcut-menu", "sth", 5 }} }, { "$1": {{ "motorola", "speed", "sth", 5 }} }, { "$1": {{ "sprint", "voice-command", "sth", 5 }} }, { "$1": {{ "motorola", "speed", "sth", 5 }} }, { "$1": {{ "iphone", "voice-clarity", "sth", 5 }} }, { "$1": {{ "samsung", "platform", "sth", 5 }} }, { "$1": {{ "t-mobile", "shortcut-menu", "sth", 5 }} }, { "$1": {{ "verizon", "voicemail-service", "sth", 5 }} } ], "t2": [ { "$2": [ 3, "John", [ { "sth": 33 }, { "sth": 44 } ] ] } ], "t4": null, "t5": null, "t7": [ 5, 10, 12.0, "sth" ], "t9": [ 3, 3, [ 9 ], null, "sth" ] } > why this change? is the order non-determistic? Not really sure about this but the order should be deterministic, I believe. The issue here was with the array_append() type computer. Somehow, the type computers for the array_append(), array_prepend() got mixed up even though it was correct in my branch. Long story short, I corrected the type computers and I had to correct this test result. https://asterix-gerrit.ics.uci.edu/#/c/2756/12/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/AListFirstTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/AListFirstTypeComputer.java: Line 45: return AUnionType.createUnknownableType(argType); > why change to unknownable here? If it's only used for reverse, sort and dis Done. On a side note, I'm just wondering then why TypeComputeUtils.getResultType() uses AUnionType.createUnknownableType() for nullable cases. https://asterix-gerrit.ics.uci.edu/#/c/2756/12/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayDistinctDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayDistinctDescriptor.java: Line 99: private final HashMap<Integer, List<AbstractPointable>> hashes; > use it.unimi.dsi.fastutil.ints.Int2ObjectMap to avoid Integer object constr Done Line 136: sameHashes = hashes.getOrDefault(hash, new ArrayList<>()); > can we reuse this ArrayList instead of creating it each time? Not sure if I got your point. The list will be created when I only need to create one. If there is no mapping, I will have to create a new list anyways. If there is a mapping, the list will not be created and I'll be reusing the list that is returned by the map. Am I missing something here? Line 141: item = new VoidPointable(); > can we use IObjectPool to reuse pointable references instead of allocating Done Line 164: storage = new ArrayBackedValueStorage(); > use IObectPool to pool storage objects? Done https://asterix-gerrit.ics.uci.edu/#/c/2756/12/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArraySortDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArraySortDescriptor.java: Line 117: storage = new ArrayBackedValueStorage(); > can we use IObjectPool to reuse storage and pointable references instead of Done https://asterix-gerrit.ics.uci.edu/#/c/2756/12/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/functions/FunctionCollection.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/functions/FunctionCollection.java: Line 393: fc.add(ArrayReverseDescriptor.FACTORY); > why are we not using code-gen versions here? Seems like all these functions Done. I was probably debugging and didn't put them back. -- To view, visit https://asterix-gerrit.ics.uci.edu/2756 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5e9c4ff6400b7fe93ca2aab0234750dc78f50659 Gerrit-PatchSet: 12 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ali Alsuliman <ali.al.solai...@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: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
