>From Wail Alkowaileet <[email protected]>: Attention is currently required from: Till Westmann, Nam Duong. Wail Alkowaileet has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17295 )
Change subject: [NO ISSUE][FUN] Implement array_swap, array_move, array_bin_search ...................................................................... Patch Set 4: (8 comments) File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/array_fun/array_binary_search/array_binary_search.3.query.sqlpp: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17295/comment/9b7491d9_a6dbc452 PS4, Line 22: FROM Can we have a test where there're multiple items with the same values to test the logic of fetching the index of the first occurrence? (I don't see one -- maybe I'm blind 😊) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17295/comment/d6f08699_bd9bdf8e PS4, Line 23: referred-topics Just a note, 'referred-topics' are not sorted sometimes File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/array_fun/array_move/array_move.3.query.sqlpp: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17295/comment/0952b70d_e164cd85 PS4, Line 20: TinySocial Add test where oldIndex > newIndex. Again, maybe I'm blind :) File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayBinarySearchDescriptor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17295/comment/23622ec9_1617530c PS4, Line 53: ArrayBinarySearchDescriptor It would good to have a JavaDoc for each new function that describes 1) the input(s) 2) the output See the following example: https://github.com/apache/asterixdb/blob/a8f889d024ac7ee58bd8414dcff4f32f39fc2cf5/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayConcatDescriptor.java https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17295/comment/d697f430_6f269661 PS4, Line 99: m Let's have a more expressive name for the member here (e.g., midIndex or resultIndex)? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17295/comment/9c5ef990_fecff119 PS4, Line 160: if (comparison == 0) { It would be better if we break the logic here as follows: - First, search for the required value - If found, then call a method (fetchFirstValue), which searches for the first occurrence of the required value. Doing so would eliminate the multiple returns and the redundant code blocks. File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArrayMoveDescriptor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17295/comment/d16fc31e_277e67ba PS4, Line 52: ArrayMoveDescriptor It seems Like array_move and array_swap share a big portion of the code. We could have an abstract class for both functions to reduce redundant code. File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/ArraySwapDescriptor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17295/comment/2e692fa1_f964c26d PS4, Line 85: oldItem I think we can use storage for both old and new. So, we can remove this -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17295 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: I2d526d90f04e62a3cc860775103254bd46d530b9 Gerrit-Change-Number: 17295 Gerrit-PatchSet: 4 Gerrit-Owner: Nam Duong <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Nam Duong <[email protected]> Gerrit-Reviewer: Wail Alkowaileet <[email protected]> Gerrit-CC: Till Westmann <[email protected]> Gerrit-Attention: Till Westmann <[email protected]> Gerrit-Attention: Nam Duong <[email protected]> Gerrit-Comment-Date: Mon, 05 Dec 2022 19:08:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
