Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug fix for order by header #2870

Merged
merged 8 commits into from
Jan 8, 2022
Merged

Conversation

makalaaneesh
Copy link
Contributor

This is a fix for #2859

Solution: Modify the toString method of the OrderByNode in the parser to selectively add "ASC"/"DESC" depending upon the OrderType. The current implementation checks for ASC type, and defaults to DESC. It does not take into account the default OrderType (i.e. when neither ASC/DESC is specified in the query - which is the case described in issue #2859)

Few additional points:

  • There seem to be other places where the check is only between ASC and DESC, ignoring the default case(for example, BoundWindowExpression::ToString()). I wasn't sure how to reproduce the bug in those cases, so I didn't include the fix in this PR. If I manage to, I will raise another PR.
  • From the documentation I read, it didn't seem possible to assert output headers using sqllogictest. If there is, pls let me know! I ended up writing a c++ test case. I wrote it in api/test_results.cpp. Pls let me know if there's a better location for that test case.

Thanks!

@Mytherin
Copy link
Collaborator

Mytherin commented Jan 8, 2022

Thanks for the PR! This looks good to me.

There seem to be other places where the check is only between ASC and DESC, ignoring the default case(for example, BoundWindowExpression::ToString()). I wasn't sure how to reproduce the bug in those cases, so I didn't include the fix in this PR. If I manage to, I will raise another PR.

In general the default case should only exist in the parsed expressions, not in the bound expressions. The reason is that during the binding the default order is converted into either ASC or DESC as specified in the default_order setting. The bound window expression could have an assertion that the order is not the default order to make this clear.

From the documentation I read, it didn't seem possible to assert output headers using sqllogictest. If there is, pls let me know! I ended up writing a c++ test case. I wrote it in api/test_results.cpp. Pls let me know if there's a better location for that test case.

We have the alias function but it doesn't seem to work correctly here. For now a C++ test is fine.

@Mytherin Mytherin merged commit 2753613 into duckdb:master Jan 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants