Skip to content

doc: add implementation guidelines for datafusion-spark #24140

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.

Sign up for GitHub

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

Merged
comphead merged 1 commit into from
Aug 20, 2026
Merged

doc: add implementation guidelines for datafusion-spark #24140

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 28 additions & 12 deletions datafusion/spark/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,39 @@ This crate is a submodule of DataFusion that provides [Apache Spark] compatible
[apache datafusion]: https://datafusion.apache.org/
[apache spark]: https://spark.apache.org/

## Testing Guide
## Implementation Guidelines

When testing functions by directly invoking them (e.g., `test_scalar_function!()`), input coercion (from the `signature`
or `coerce_types`) is not applied.
When implementing these functions, you can check if there are existing implementations
in the [Sail] or [Comet] projects first. If you do port functionality from these
sources, make sure to port over the corresponding tests too, to ensure correctness
and compatibility.

Therefore, direct invocation tests should only be used to verify that the function is correctly implemented.
### `simplify()`

@andygrove andygrove Aug 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is a challenge here on how to write sql logic tests to test both the simplify and the invoke_with_args implementations.

Do you have suggestions on how to address this?

@Jefffrey Jefffrey Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is one of the cases where Rust unit tests would be needed; though perhaps it also suggests that maybe we should stick to invoke_with_args only, and avoid simplify altogether especially in cases where it is an unconditional rewrite 🤔

@comphead comphead Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ideally we need to guarantee parity between simplify and invoke_with_args however there is no flag the spark function is simplified to make a decision.

We prob need to come up with solution to disable optimizer rules in slt.
Spark already has this through SQLConf.OPTIMIZER_EXCLUDED_RULES I suppose we need similar for DataFusion, not only for Spark but in general.

Having the mechanism it would be easier to solve questions like @andygrove mentioned. If you okay, I can come up with draft PR

@Jefffrey Jefffrey Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This sounds great, especially for the datafusion UDFs; itll be easier to have tests in slts that can run with & without simplify to ensure they have same result, instead of having those tests split between slt and rust unit test


Please be sure to add additional tests beyond direct invocation.
For more detailed testing guidelines, refer to the [Spark SQLLogicTest README].
DataFusion functions allow you to implement `simplify()` which can let you rewrite
the function call during logical optimization, theoretically allowing you to avoid
implementing physical execution via `invoke_with_args()` if the rewrite is unconditional
(e.g. rewrite to an arithmetic operation).

## Implementation References
**However, `invoke_with_args()` must always be implemented for functions in this
crate.** This is because downstream users such as Comet rely on DataFusion for physical
execution, and not logical planning/optimization. That means if a function doesn't
have a physical implementation (`invoke_with_args()`) it is not usable by Comet.

When implementing Spark-compatible functions, you can check if there are existing implementations in
the [Sail] or [Comet] projects first.
If you do port functionality from these sources, make sure to port over the corresponding tests too, to ensure
correctness and compatibility.
### Supported types

The functions in this crate need only support input types available to Spark; that
is, they do not need to handle unsigned types or types such as `Float16` or `Decimal64`.

[spark sqllogictest readme]: ../sqllogictest/test_files/spark/README.md
[sail]: https://github.com/lakehq/sail
[comet]: https://github.com/apache/datafusion-comet

## Testing Guidelines

Prefer adding tests via SQLLogicTests where possible, see the [Spark SQLLogicTest README].
Resort to adding tests as Rust unit tests where it is impossible or difficult to
test via SLT. This is because direct invocation via Rust skips steps such as input
coercion, and is usually more verbose in the setup needed to pass data in (and
assert output data).

[spark sqllogictest readme]: ../sqllogictest/test_files/spark/README.md
Loading