duckdb-r icon indicating copy to clipboard operation
duckdb-r copied to clipboard

rel_order accepts sort order as an argument

Open toppyy opened this issue 9 months ago • 0 comments

Related to duckdblabs/duckplyr#92

Changes:

  1. rapi_rel_order now accepts a vector of boolean values that describe whether each expression should be sorted in ascending order
  2. rel_order checks that the number of expressions and boolean values match or defaults to TRUE if a null-value is passed
  3. tests for the relation api test the rel_order(), not rapi_rel_order(). The tests break without this change as the default value is handled in rel_order.

I will mark this as a draft due to these notes:

  • This does not work for nested expressions. For example, assume someone writes arrange(someotherfunction(desc(column))) in duckplyr. I don't know if this is an issue as the interpretation of such an expression in terms of sort order is a bit ambiguous(?).
    • note: this problem exists also without changes proposed here?
  • I was unsure what function should to check that length(orders) == length(ascending). It is now rel_order(), rapi_rel_order(). As a result, using rapi_rel_order directly might result indexing out of array bounds. Perhaps the check should be in rapi_rel_order as the vector is indexed there?
  • Related to the previous note. Should we aim to test rel_order() or rapi_rel_order? There are examples of both for other relation expressions

toppyy avatar May 12 '24 07:05 toppyy