Repository navigation
feat(cli): add opt-in Spark function registration - #24179
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24179 +/- ##
==========================================
- Coverage 81.17% 81.17% -0.01%
==========================================
Files 1109 1109
Lines 388038 388038
Branches 388038 388038
==========================================
- Hits 314992 314977 -15
- Misses 54507 54517 +10
- Partials 18539 18544 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
neilconway
left a comment
There was a problem hiding this comment.
Thanks for this! High-level motivation and approach seems fine to me.
I wonder whether --spark should also set the SQL dialect to "spark". Probably?
Agreed and addressed it as well @neilconway |
|
@goutamadwant Can you check whether the current state of the PR is accurate? I don't see the changes to the SQL dialect, for example. |
|
Thanks for catching this @neilconway changes were locally but forgot to push it. latest commit has these changes now. |
neilconway
left a comment
There was a problem hiding this comment.
Looks good overall!
It might be useful to update datafusion/sqllogictest/test_files/spark/README.md and datafusion/spark/README.md to mention this feature.
Can you update the PR description for the latest version of the PR? e.g., spark dialect.
neilconway
left a comment
There was a problem hiding this comment.
Looks good overall!
It might be useful to update datafusion/sqllogictest/test_files/spark/README.md and datafusion/spark/README.md to mention this feature.
Can you update the PR description for the latest version of the PR? e.g., spark dialect.
|
Thanks @neilconway addressed this with latest push . |
timsaucer
left a comment
There was a problem hiding this comment.
Thank you for this! I was just looking at the issue list to see if we had an open issue, but we already that and an approved PR to resolve it! This is great.
| // enable dynamic file query | ||
| let ctx = SessionContext::new_with_config_rt(session_config, runtime_env) | ||
| .enable_url_table(); | ||
| let ctx = SessionContext::new_with_state(state_builder.build()).enable_url_table(); |
There was a problem hiding this comment.
This is annoying, because we end up creating a session to just turn it back into a builder to add in the url table catalog and then turn it back into a session. Changing it would be a larger PR, so I suppose this is fine.
|
Thanks @goutamadwant @timsaucer ! The session builder cleanup would be worth looking at in a follow-on PR. I notice the GitHub issue mentioned printing something on CLI startup to indicate that Spark mode is enabled; wdyt? |
## Which issue does this PR close? - Closes apache#24146. ## Rationale for this change `datafusion-cli` does not currently enable the Spark SQL dialect or register the `datafusion-spark` function library, so Spark-compatible syntax and functions cannot be explored interactively. Enabling them by default could override core DataFusion functions with the same names and change existing CLI behavior. ## What changes are included in this PR? - Add a `--spark` CLI flag. - Select the Spark SQL dialect while constructing the session configuration. - Register Spark-compatible scalar, aggregate, and window functions only when the flag is enabled. - Add CLI integration coverage for both default and opt-in behavior. - Document the flag and its function-override behavior in the CLI and Spark documentation. ## Are these changes tested? Yes. The following checks pass: - `cargo test -p datafusion-cli --test cli_integration spark_features_require_opt_in` - `cargo test -p datafusion-cli --all-targets` - `cargo fmt --all -- --check` - `cargo clippy --all-targets --all-features -- -D warnings` - `./ci/scripts/doc_prettier_check.sh` ## Are there any user-facing changes? Yes. Passing `--spark` enables the Spark SQL dialect, Spark expression planner, and Spark-compatible functions for the CLI session. The option is disabled by default, so existing CLI behavior remains unchanged.
…on-spark) (apache#27337) ## Which issue does this PR close? - Part of #27103. ## Rationale for this change As agreed in #27103, `datafusion-spark` is moving to the new [apache/datafusion-spark](https://github.com/apache/datafusion-spark) repository, along with its git history. `datafusion-spark` 56.0.0 is the last release from this repository (`branch-56` keeps the crate), and 57.0.0 will be released from the new one. This stays a draft until the code has been imported into apache/datafusion-spark. Open PRs that touch these files have a comment pointing to #27103. ## What changes are included in this PR? - Removes the crate (`datafusion/spark`), its sqllogictest files (`datafusion/sqllogictest/test_files/spark`) and the `audit-datafusion-spark-expression` agent skill, all of which move to the new repository. - `datafusion-cli`: removes the `--spark` flag and its test. Without the crate, the flag could only switch the SQL dialect, which `SET datafusion.sql_parser.dialect = 'spark'` already does. The CLI builds its context with `SessionContext::new_with_config_rt` again, as it did before apache#24179. - `datafusion-sqllogictest`: removes the Spark session setup for files under `spark/`. The Spark result formatting in the `DataFusion` engine stays, so the new repository can reuse the published engine. - CI: removes the `cargo check datafusion-spark features` job, its required check in `.asf.yaml`, its entries in `ci/scripts/check_rust_workflow_skips.py`, and the labeler rule. - Docs: points the Spark functions page and the contributor how-to at the new repository, removes `--spark` from the CLI usage page and the crate from the release publish steps, and adds a 57.0.0 upgrade note. `SparkOptions` (`datafusion.spark.*`) stays in `datafusion-common`, because `datafusion-spark` reads it. Moving it into the crate as a `ConfigExtension` can be a follow-up. ## What is the testing strategy for this PR? This PR removes code and adds no tests. I ran these locally with stable Rust 1.97, because the pinned 1.99.1 isn't available on my machine: - `cargo check --workspace --all-targets` - both clippy passes from `ci/scripts/rust_clippy.sh` with `-D warnings`. I allowed `clippy::from_iter_instead_of_collect`, `unfulfilled_lint_expectations` and `unknown_lints`, which only fire on 1.97 and only in code this PR doesn't touch. - `cargo fmt`, taplo, typos, prettier, the generated and examples docs checks, and the `.asf.yaml` and workflow-skip validators - the extended test suite from `CLAUDE.md`: 13,332 passed, 0 failed, including all 304 sqllogictest files - `cargo test --profile ci -p datafusion-cli`: 115 passed, 0 failed ## Are there any user-facing changes? Yes: - `datafusion-cli --spark` is removed. - `datafusion-spark` is published from apache/datafusion-spark starting with 57.0.0. The crate name doesn't change. --------- Co-authored-by: Claude Opus 5.5 <[email protected]>
Which issue does this PR close?
Rationale for this change
datafusion-clidoes not currently enable the Spark SQL dialect or register thedatafusion-sparkfunction library, so Spark-compatible syntax and functions cannot be explored interactively. Enabling them by default could override core DataFusion functions with the same names and change existing CLI behavior.What changes are included in this PR?
--sparkCLI flag.Are these changes tested?
Yes. The following checks pass:
cargo test -p datafusion-cli --test cli_integration spark_features_require_opt_incargo test -p datafusion-cli --all-targetscargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warnings./ci/scripts/doc_prettier_check.shAre there any user-facing changes?
Yes. Passing
--sparkenables the Spark SQL dialect, Spark expression planner, and Spark-compatible functions for the CLI session. The option is disabled by default, so existing CLI behavior remains unchanged.