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

test: Port order.rs tests to sqllogictest #8857

Merged
merged 3 commits into from
Jan 15, 2024

Conversation

simicd
Copy link
Contributor

@simicd simicd commented Jan 13, 2024

Which issue does this PR close?

Closes #8205

Rationale for this change

Migrate remaining Rust unit tests in order.rs to sqllogictest

What changes are included in this PR?

  • Move unit tests from order.rs to order.slt
  • Move parquet file to git submodule

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!

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jan 13, 2024
Copy link
Contributor

@alamb alamb left a 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.

datafusion/sqllogictest/test_files/order.slt Outdated Show resolved Hide resolved
@simicd
Copy link
Contributor Author

simicd commented Jan 15, 2024

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?

@alamb
Copy link
Contributor

alamb commented Jan 15, 2024

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

Copy link
Contributor

@alamb alamb left a 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

datafusion/sqllogictest/test_files/order.slt Outdated Show resolved Hide resolved
Copy link
Contributor

@alamb alamb left a 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

@alamb alamb merged commit aff7094 into apache:main Jan 15, 2024
22 checks passed
@simicd simicd deleted the test/order-sqllogictest branch January 15, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port tests in order.rs to sqllogictest
2 participants