>From Nam Duong <[email protected]>: Attention is currently required from: Wail Alkowaileet, Till Westmann. Nam Duong 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 6: Code-Review+1 (10 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/9ad5f5cc_41537563 PS4, Line 22: FROM > Can we have a test where there're multiple items with the same values to test > the logic of fetching […] Done. Don't worry, you're not blind 😄 - i just completely forgot to test it - will implement the tests 😊 https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17295/comment/61279da2_ada077f3 PS4, Line 23: referred-topics > Just a note, 'referred-topics' are not sorted sometimes Ack 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/2069c086_75ab96bb PS4, Line 20: TinySocial > Add test where oldIndex > newIndex. […] i think when writing the tests, since -1 = last index and -3 = (last index-2), i thought it would be suffice for testing the oldindex > newindex case. looking back, i realise it might have just been a lazy excuse! sorry, will add test cases now 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/938e9f08_789ca922 PS4, Line 53: ArrayBinarySearchDescriptor > It would good to have a JavaDoc for each new function that describes […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17295/comment/dc3fab11_a81e584a PS4, Line 99: m > Let's have a more expressive name for the member here (e.g. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17295/comment/e5464844_8beb9833 PS4, Line 137: listType.isListType > Let's restrict this function and also array_swap and array_move to arrays > only -- i.e. […] Ack https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17295/comment/66130374_53665420 PS4, Line 160: if (comparison == 0) { > It would be better if we break the logic here as follows: […] So I've implemented a method to fetch the first occurrence of the search value, but within the new method, there are still multiple returns due to multiple `if currIndex == 0` checks. Was just wondering if this is still okay? The evaluate() method is much cleaner now though, so thank you for the advice! 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/97aa250b_acda3c20 PS4, Line 52: ArrayMoveDescriptor > It seems Like array_move and array_swap share a big portion of the code. […] Was planning to do an extraction soon to do just that! Thanks for all the comments :) Will be working on this soon https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17295/comment/56993ebb_82952693 PS4, Line 160: AUnorderedListType > We need only to consider arrays and exclude multiset Done 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/50c5813a_3a608286 PS4, Line 85: oldItem > I think we can use storage for both old and new. […] Ack -- 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: 6 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: Wail Alkowaileet <[email protected]> Gerrit-Attention: Till Westmann <[email protected]> Gerrit-Comment-Date: Fri, 09 Dec 2022 00:17:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Wail Alkowaileet <[email protected]> Gerrit-MessageType: comment
