feat: add `sql` feature to make sql planning optional #17332

timsaucer · 2025-08-26T18:57:53Z

Which issue does this PR close?

Rationale 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 the core crate and makes sqlparser an optional dependency. The rest of the changes are to account for the fact that some of the structs in sqlparser 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 add sql to their Cargo.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:

cargo build --release -p datafusion --lib --no-default-features  374.93s user 11.45s system 800% cpu 48.276 total
-rw-r--r--@ 1 tsaucer  staff    10M Aug 27 08:15 target/release/libdatafusion.rlib

cargo build --release -p datafusion --lib --no-default-features --features sql  453.58s user 12.40s system 761% cpu 1:01.21 total
-rw-r--r--@ 1 tsaucer  staff    17M Aug 27 08:17 target/release/libdatafusion.rlib

cargo build -p datafusion --lib --no-default-features  172.85s user 13.35s system 637% cpu 29.189 total
-rw-r--r--@ 1 tsaucer  staff    49M Aug 27 08:20 target/debug/libdatafusion.rlib

cargo build -p datafusion --lib --no-default-features --features sql  210.16s user 14.26s system 471% cpu 47.625 total
-rw-r--r--@ 1 tsaucer  staff    91M Aug 27 08:19 target/debug/libdatafusion.rlib

Net results:

Release mode without SQL: 48.276 seconds, 10M library
Release mode with SQL: 61.21 seconds, 17M library
Debug mode without SQL: 29.189 seconds, 49M library
Debug mode with SQL: 47.625 seconds, 91M library

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.

crepererum · 2025-08-28T09:19:24Z

I've adjusted the description because I think that also closes #17258 🥳

crepererum · 2025-08-28T09:23:39Z

.github/workflows/rust.yml

          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


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 reviewed the existing coverage and it seems like the current pattern is to test features in CI here:

https://github.com/apache/datafusion/blob/49d49fd92dddf55bfb22787fea17dda1a698dc4d/.github/workflows/rust.yml#L188-L187

I took the liberty of adding the appropriate entry for the sql feature in this PR and documeting the feature

mbutrovich · 2025-09-03T15:06:57Z

I was playing with this PR this morning to see if Comet could use it, and it looks like datafusion-sql is brought in with the nested_expressions feature (which Comet uses). I wonder if we can disentangle that dependency.

findepi · 2025-09-04T13:49:04Z

Closes Make sqlparser dependency optional #17258

❤️ ❤️ ❤️ ❤️ ❤️

alamb · 2025-09-05T18:23:26Z

I was playing with this PR this morning to see if Comet could use it, and it looks like datafusion-sql is brought in with the nested_expressions feature (which Comet uses). I wonder if we can disentangle that dependency.

that would be great

timsaucer · 2025-09-05T19:25:36Z

I was playing with this PR this morning to see if Comet could use it, and it looks like datafusion-sql is brought in with the nested_expressions feature (which Comet uses). I wonder if we can disentangle that dependency.

I did a quick check and with the latest push + comet I no longer have any sql dependency in the cargo lock file! I did have to make a change in comet, which was to set datafusion-functions-nested = { path="../../../datafusion/datafusion/functions-nested", default-features = false } in native/core/Cargo.toml.

mbutrovich · 2025-09-05T19:28:44Z

I did a quick check and with the latest push + comet I no longer have any sql dependency in the cargo lock file! I did have to make a change in comet, which was to set datafusion-functions-nested = { path="../../../datafusion/datafusion/functions-nested", default-features = false } in native/core/Cargo.toml.

I'll give this a shot, thanks @timsaucer!

…plan in an effort to remove the hash functions from the final wasm blob. Probably need to revert or put under a feature flag.

…ntifiers

timsaucer · 2025-09-05T19:37:43Z

I had to rebase, so you might want to pull again

alamb

Thank you @timsaucer

Sorry for the delay in this PR. I merged up from main to resolve some conflicts and added a new test and docs. Hopefully it is good to go now.

Thanks again!

alamb · 2025-09-16T11:49:14Z

.github/workflows/rust.yml

          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


I reviewed the existing coverage and it seems like the current pattern is to test features in CI here:

https://github.com/apache/datafusion/blob/49d49fd92dddf55bfb22787fea17dda1a698dc4d/.github/workflows/rust.yml#L188-L187

I took the liberty of adding the appropriate entry for the sql feature in this PR and documeting the feature

mbutrovich

I didn't see a big win from the final Comet binary size, but I think any encapsulation improvements are welcome. Thanks @timsaucer!

timsaucer · 2025-09-16T15:21:33Z

I didn't see a big win from the final Comet binary size, but I think any encapsulation improvements are welcome. Thanks @timsaucer!

Right - I don't think you will see a binary difference, especially in release mode. But hopefully you will get some improvements in build time and number of dependencies.

alamb · 2025-09-16T17:22:13Z

🎉

findepi · 2025-09-17T14:05:43Z

datafusion/common/src/utils/mod.rs

 }

+#[cfg(not(feature = "sql"))]
+pub(crate) fn parse_identifiers(s: &str) -> Result<Vec<String>> {


I don't think we should have two different, divergent implementations of parse_identifiers and parse_identifiers_normalized. This leads to unpredictable runtime behavior when integrating different components using DataFusion (one using sql feature and one not).

If the new implementation is equivalent to sqlparser-based, it should be the only implementation.

If it is not, it should not exist. All the APIs using it should be guarded by sql feature flag.

Column::from_qualified_name result is potentially feature-set dependent #17623

findepi · 2025-09-17T14:07:52Z

datafusion/expr/src/expr.rs

+#[derive(Clone, PartialEq, Eq, PartialOrd, Hash, Debug)]
+#[cfg(not(feature = "sql"))]
+pub struct ExceptSelectItem {
+    pub first_element: Ident,
+    pub additional_elements: Vec<Ident>,
+}
+#[cfg(not(feature = "sql"))]
+impl Display for ExceptSelectItem {
+    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
+        write!(f, "EXCEPT ")?;
+        if self.additional_elements.is_empty() {
+            write!(f, "({})", self.first_element)?;
+        } else {
+            write!(
+                f,
+                "({}, {})",
+                self.first_element,
+                display_comma_separated(&self.additional_elements)
+            )?;
+        }
+        Ok(())
+    }
+}


The expr crate should become un-dependent from sqlparser::ast types, so these copies should be the only types we use.

Is there a follow-up issue planned to cleanup these duplicate types?

Filed

Clean up duplicate types in sql module #17696

timsaucer self-assigned this Aug 26, 2025

timsaucer marked this pull request as ready for review August 27, 2025 11:18

mbutrovich self-requested a review August 27, 2025 15:05

crepererum reviewed Aug 28, 2025

View reviewed changes

timsaucer added 8 commits September 5, 2025 15:29

Intermediate work on setting up sql feature flags throughout repo

dc53f30

More intermediate work, but does not yet compile

6165430

Working through more points of friction to remove sql as feature

7c466de

Switch optimizer to compare full logical plan instead of hash of the …

4a6ca20

…plan in an effort to remove the hash functions from the final wasm blob. Probably need to revert or put under a feature flag.

Corrections after rebase

f2a9889

Resolve errors and clippy warnings after rebase

e3ad2f1

Remove unused imports when not using sql feature

00e2202

Working through more feature gating

36272cf

timsaucer added 7 commits September 5, 2025 15:30

Working through more feature gating

6139746

wasmtest required sql

d5b04ea

working on CI

ddaf8a3

Update docs

95eff0d

Remove some duplicate code by doing a bit of sql-like parsing of ided…

28a0d01

…ntifiers

Conditionally import sql features

d40eb8b

Set default features to false so we do not always pull in sql

7066b0e

timsaucer force-pushed the feat/sql-as-feature branch from c5852b7 to 7066b0e Compare September 5, 2025 19:37

alamb added 3 commits September 16, 2025 07:45

Merge remote-tracking branch 'apache/main' into feat/sql-as-feature

993ef00

Add check for just sql feature

fdac9bb

Update readme for features

495ad57

github-actions bot added documentation Improvements or additions to documentation and removed catalog Related to the catalog crate labels Sep 16, 2025

alamb changed the title ~~feat: make sql an optional feature~~ feat: add sql feature Sep 16, 2025

alamb changed the title ~~feat: add sql feature~~ feat: add sql feature to make sql planning optional Sep 16, 2025

alamb approved these changes Sep 16, 2025

View reviewed changes

alamb added 2 commits September 16, 2025 10:24

Taplo format

80e0436

Merge remote-tracking branch 'apache/main' into feat/sql-as-feature

ce6cef2

mbutrovich approved these changes Sep 16, 2025

View reviewed changes

mbutrovich merged commit 0181c79 into apache:main Sep 16, 2025
30 checks passed

findepi reviewed Sep 17, 2025

View reviewed changes

Jefffrey mentioned this pull request Sep 18, 2025

minor: Ensure datafusion-sql package dependencies have sql flag #17644

Merged

alamb mentioned this pull request Sep 21, 2025

Clean up duplicate types in sql module #17696

Open

timsaucer deleted the feat/sql-as-feature branch October 13, 2025 13:24

feat: add sql feature to make sql planning optional #17332

feat: add sql feature to make sql planning optional #17332

Uh oh!

Conversation

timsaucer commented Aug 26, 2025 • edited by crepererum Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Additional context

Uh oh!

crepererum commented Aug 28, 2025

Uh oh!

crepererum Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

mbutrovich commented Sep 3, 2025

Uh oh!

findepi commented Sep 4, 2025

Uh oh!

alamb commented Sep 5, 2025

Uh oh!

timsaucer commented Sep 5, 2025

Uh oh!

mbutrovich commented Sep 5, 2025

Uh oh!

timsaucer commented Sep 5, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

timsaucer commented Sep 16, 2025

Uh oh!

Uh oh!

alamb commented Sep 16, 2025

Uh oh!

findepi Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

findepi Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

findepi Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

feat: add `sql` feature to make sql planning optional #17332

feat: add `sql` feature to make sql planning optional #17332

timsaucer commented Aug 26, 2025 •

edited by crepererum

Loading

alamb Sep 21, 2025 •

edited

Loading