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: Introduce datafusion engine for executing sqllogictest. #895

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

liurenjie1024
Copy link
Contributor

Second part of #581.

Introducing datafusion engine for executing sqllogictest.

@liurenjie1024 liurenjie1024 requested review from Fokko, sdd and Xuanwo January 18, 2025 09:39
Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally make sense to me, i'll defer to someone with more experience in rust.

This follows the datafusion/datafusion/sqllogictest tests suite.
Do you think its worth mentioning that conversion.rs and normalize.rs are both copied over from datafusion/datafusion/sqllogictest?

Is it possible to take datafusion_sqllogictest as a dependency instead of copying over the code?

@liurenjie1024
Copy link
Contributor Author

This follows the datafusion/datafusion/sqllogictest tests suite.
Do you think its worth mentioning that conversion.rs and normalize.rs are both copied over from datafusion/datafusion/sqllogictest?

I've synced with @Fokko and I'll add some notice in LICENSE as what we did in pyiceberg.

Is it possible to take datafusion_sqllogictest as a dependency instead of copying over the code?

Currently impossible because it's not published as crates.

Copy link
Contributor

@sdd sdd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me 👍🏼

Copy link
Contributor

@xxchan xxchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to take datafusion_sqllogictest as a dependency instead of copying over the code?

+1 on this. It seems we don't need any customization (except for ctx.register_catalog) on the test driver, so it's not economical to maintain a copy. cc @alamb for opinions. We could also consider use a git dependency for now.

@liurenjie1024
Copy link
Contributor Author

Is it possible to take datafusion_sqllogictest as a dependency instead of copying over the code?

+1 on this. It seems we don't need any customization (except for ctx.register_catalog) on the test driver, so it's not economical to maintain a copy. cc @alamb for opinions. We could also consider use a git dependency for now.

I have concerns as using a git dependency, with this approach datafusion community is not treating it as a library. I'm quite interested if datafusion community could consider exposing it as a crate. I've raised discussion, welcome to discuss there!

@alamb
Copy link

alamb commented Jan 22, 2025

I published datafusion-sqllogictest to crates.io (details here apache/datafusion#14229 (comment)) . Let us know if you have any problems!

@liurenjie1024
Copy link
Contributor Author

I published datafusion-sqllogictest to crates.io (details here apache/datafusion#14229 (comment)) . Let us know if you have any problems!

It looks great! I'll refator the pr and have a try, thanks!

@Xuanwo
Copy link
Member

Xuanwo commented Jan 23, 2025

Thank you @liurenjie1024 for working on this. I'm going to convert this PR to draft instead.

@Xuanwo Xuanwo marked this pull request as draft January 23, 2025 15:27
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.

6 participants