-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
test: Port order.rs
tests to sqllogictest
#8857
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @simicd 🙏 -- this looks great. Your contribution is very much apprecaited.
I left a comment about how to test for ORDER BY in table definition.
Hi @alamb, thanks a lot for your review and the tip, that worked! I added additional comments above. I saw that the previous CI run failed. I assume this happend because I moved the parquet test file from the main repo into the parquet-testing submodule. Does that mean I have to raise a PR in this repo first and get it merged? |
Yes, if you wanted to move it to parquet-testing you would need to make the PR in that repo first However, I think you could simply leave the file in the main repo (not put it in parquet-testing) and simply adjust the path location |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @simicd -- this is looking very close -- I think we can avoid having to change parquet-data (which is shared between different parquet implementations) that would be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome -- thank you so much @simicd
Which issue does this PR close?
Closes #8205
Rationale for this change
Migrate remaining Rust unit tests in
order.rs
to sqllogictestWhat changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
No
Hi everyone, this is my first PR in the arrow-datafusion project (I have previously contributed a few smaller PRs to the arrow-datafusion-python repository). I chose to tackle one of the issues tagged with
good first issue
as I thought it would be a good way to start contributing here. I would greatly appreciate any feedback & suggestions. Thank you in advance for taking the time to review my contribution!