-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: make sql an optional feature #17332
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
base: main
Are you sure you want to change the base?
Conversation
…plan in an effort to remove the hash functions from the final wasm blob. Probably need to revert or put under a feature flag.
I've adjusted the description because I think that also closes #17258 🥳 |
@@ -579,7 +579,7 @@ jobs: | |||
with: | |||
rust-version: stable | |||
- name: Run datafusion-common tests | |||
run: cargo test --profile ci -p datafusion-common --features=pyarrow | |||
run: cargo test --profile ci -p datafusion-common --features=pyarrow,sql |
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.
Should we extend the CI to also verify that ALL crates at least cargo check
with and without sql support? This YAML file has a few examples/listings of feature tests with cargo check
.
I was playing with this PR this morning to see if Comet could use it, and it looks like |
❤️ ❤️ ❤️ ❤️ ❤️ |
Which issue does this PR close?
sqlparser
dependency optional #17258Rationale for this change
For some users, adding in sqlparser greatly increases build times for their projects even though they are not using sql. This feature makes
sql
an optional dependency to improve build times and reduce the number of dependencies.What changes are included in this PR?
Adds in a
sql
feature in thecore
crate and makessqlparser
an optional dependency. The rest of the changes are to account for the fact that some of the structs insqlparser
are used throughout the repository.Are these changes tested?
Ideally this is effectively a no-op change. By enabling
sql
(or simply using the default features) there is no change and all tests should run as before. I have also run all tests with the--no-default-features
flag.Are there any user-facing changes?
There is one change that may be impactful to some users. By adding in the
sql
feature, if users are not using default features of the datafusion core crate, they will need to addsql
to theirCargo.toml
.Additional context
When building on a mac m4, running a
cargo clean
and then checking time and build size these are my local results for just building the core datafusion library:Net results:
The absolute time numbers aren't so bad when you build just this library by itself, but as part of larger projects it has a compounding effect.