objectiv
WIP: Bach - extracted order_by_expression function
Based on review comment https://github.com/objectiv/objectiv-analytics/pull/1062#discussion_r928295212
@KathiaBarahona does this match what you had intended?
lgtm, tho I'm not sure if
get_order_by_expressionshould be in partitioning module.
It's used by window functions and an aggregate function, so this seemed like an okayish place to put it. Do you have a concrete proposal that you think is better?
desc + nulls first is wrong
What do you mean by wrong? Is this different from what Pandas does, or how we did it in the past, or ... ?
desc + nulls first is wrong
What do you mean by wrong? Is this different from what Pandas does, or how we did it in the past, or ... ?
( I'm dumb, was replying on your comment)
See https://github.com/objectiv/objectiv-analytics/pull/1070#discussion_r934263974
when nulls_last is False and order_by.asc is False, the final expression in the order by clause is setting ORDER BY COLUMN desc nulls last
is yielding the same expression when having nulls_last is True and order_by.asc is False
desc + nulls first is wrong
What do you mean by wrong? Is this different from what Pandas does, or how we did it in the past, or ... ?
( I'm dumb, was replying on your comment)
See #1070 (comment)
when
nulls_last is Falseandorder_by.asc is False, the final expression in the order by clause is settingORDER BY COLUMN desc nulls lastis yielding the same expression when having
nulls_last is Trueandorder_by.asc is False
Let's discuss tomorrow after stand-up. I am not quite following what you mean. To me it looks like the behaviour of the function matches what the docstring says it does, and the test for SeriesString keeps working. I agree that this might change things for window functions, but I believe your PR fixes that?
Closing this PR. PR https://github.com/objectiv/objectiv-analytics/pull/1089 contains all commits in this branch, with a lot more refinement added