feat: Support descending sort for character and other non-numeric data

[toppyy]

Closes #92.

Creating a desc macro feels a bit like a work-around as it is created for the sole purpose of not raising a function-not-found -error. Couldn't think of an easy alternative. What do you think @krlmlr ?

[krlmlr]

Thanks. We need to adopt the translation of desc() and remove the macro. The translation lives in relational.R in the rel_translate() function. It's not very clean, and this is a shame because this is the space where we want and need to improve. Happy to guide you through and improve the code organization step by step.

[toppyy]

Thanks! Before diving into the code organization of the rel_translate() function, I thought about what to translate desc into. To me it seems that one solution is to translate it into an expression reference of it's first argument with an attribute dictating the descending sort order.

So, for example, translating the expression desc(some_column) would return something like:

Then we could use the attribute "desc" to derive the sort order in rel_order.duckdb_relation()

Would this make sense?

[krlmlr]

Thanks, good catch. I think we can just special-case desc() in arrange(), this doesn't need to live in rel_translate() . In other words, failing to translate desc() in all cases except when used as the surrounding call in an arrange() expression is fine.

[toppyy]

I pushed a new version that handles calls to desc() in arrange(). We extract the sort order first and then manipulate the arguments to arrange so that calls to desc() are removed before passing the arguments for translation.

The arguments ("dots") is iterated twice which is not ideal, but I found it to be cleaner than manipulating dots-argument in place.

[toppyy]

Do you have comments on the above @krlmlr ? If this was already on your todo-list, sorry for the unnecessary ping.

[krlmlr]

Thanks for the ping. I was waiting for duckdb 1.0.0 to hit CRAN, which now happened. We'll need duckdb (>= 1.0.0) in DESCRIPTION, I've started the checks to see if the new code passes checks.

[krlmlr]

Thanks, this looks workable, just a few remarks on style.

[krlmlr]

• map_lgl()
• is.call() perhaps?
• Avoid return() as last statement
• Extract this and the next map into a function that returns a data frame (or a list with two parallel vectors) and uses ... a for loop?

[toppyy]

Thanks! I refactored the extraction of sort order and manipulation of dots into a separate function.

Also the parameter ascending in rel_order() now has NULL as a default argument (the tests for previous commit failed due to not having a default argument)

[krlmlr]

Thanks, let me play with it and finish it.

[krlmlr]

Do we need to use the original environment here?

[toppyy]

Thanks! Good point.

[krlmlr]

Thanks!