[substrait] Build basic test suite to validate produced Substrait plans

Is your feature request related to a problem or challenge?

In #12244 we found that DataFusion was producing invalid Substrait plans. The datafusion-substrait crate has quite a bit of tests but none that actually verify plans are valid according to the https://github.com/substrait-io/substrait-validator.

Describe the solution you'd like

Add a test suite to the datafusion-substrait crate that uses https://github.com/substrait-io/substrait-validator to validate plans. We can start small and just add a test that would've failed prior to #15011.

Describe alternatives you've considered

No response

Additional context

No response

Related ticket perhaps:

substrait generated by Apache Calcite does not run in DataFusion #14831

I started looking into running the sqllogictests through Substrait, converting the original SQL to Substrait through Isthmus.
The main flow works, there are "only" 105 failures (some real ones, but probably quite a few due to my wonky setup.)

Quick update: 105 failures was actually the number of test files that fail so... all of them.

I've managed to get roughly 1/3 of the select queries to produce valid Substrait plans, but then most of them fail to execute because of type issues or missing functions. And that's after transforming all SQL strings to double quote identifiers to avoid casing and reserved names errors.

So I don't believe automatically running the full sqllogictests through Substrait is feasible today. A more realistic goal would be to have a few hand-crafted queries (TPCH queries as a start for instance), and assert that their SQL and Substrait results match.

@geoffreyclaude

So I don't believe automatically running the full sqllogictests through Substrait is feasible today. A more realistic goal would be to have a few hand-crafted queries (TPCH queries as a start for instance), and assert that their SQL and Substrait results match.

Thanks @geoffreyclaude -- I think we have the basic TPCH plans covered in https://github.com/apache/datafusion/blob/main/datafusion/substrait/tests/substrait_integration.rs

https://github.com/apache/datafusion/tree/main/datafusion/substrait/tests/testdata/tpch_substrait_plans specifically that @vbarua worked on

One idea that comes to mind is to be able to run the sqllogictests in "substrait roundrip" mode. Upon building a logical DataFusion plan, it would be converted to Substrait and then back to DataFusion logical for each sqllogictest statement.

I played around with this idea and it seems to be catching existing issues like #15855.

Some advantage of this is that it's a high-coverage/low-effort solution, and that it tests both consuming and producing Substrait plans, but on the other hand the tests would be biased towards how datafusion-substrait produces plans, which might not be exactly the same as other systems.

It is a good idea -- another potential issue is that it would effectively "tax" other features in the sense that writing tests for unrelated features might trigger a substrait bug that was unrelated to the test being written

If we want to add this round trip method I think we should also add a per-test way to disable the round trip testing so we could merge the unrelated PR (and file a ticket to fix the substrait issue as a follow on issue)

Basically I think we should avoid blocking other development as much as possible

another potential issue is that it would effectively "tax" other features in the sense that writing tests for unrelated features might trigger a substrait bug that was unrelated to the test being written

Fortunately sqllogictest-rs has support for this via engine names. Having a separate implementation of sqllogictest::AsyncDB with a "DataFusionSubstraitRoundrip" engine name should allow us to flag individual sqllogictests with skipif DataFusionSubstraitRoundrip.

Although I imagine that if we go down this path, some time will pass until this new testing mode can be enabled in the CI.

One idea that comes to mind is to be able to run the sqllogictests in "substrait roundrip" mode. Upon building a logical DataFusion plan, it would be converted to Substrait and then back to DataFusion logical for each sqllogictest statement.

I've explored this idea in

Add --substrait-round-trip option in sqllogictests gabotechs/datafusion#6

which builds on top of

Support filtering specific sqllogictests identified by line number #16029

And I'm quite happy with the result. It does seem like this approach is catching relevant bugs, and the idea of creating scoped issues with a reproducer as portable as

 cargo test --test sqllogictests -- --substrait-round-trip aggregate:1522

leads me to think that it can incentivize people to contribute fixes

And I'm quite happy with the result. It does seem like this approach is catching relevant bugs, and the idea of creating scoped issues with a reproducer as portable as

Perhaps what we can do is to

Add a new CI job / phase of an existing job to run sqllogictests with substrait validation
Add some way to ignore a test / exclude it from running in substrait validation mode (to avoid blocking PRs that are unrelated but find a substrait bug with new tests)
File a ticket with the goal of "get all tests running in substrait validation mode"

That sounds reasonable, although given the current state there's a lot of ignoring to be done in the Substrait validation mode, some numbers here: currently out of 7302 query sqllogictest statements, only 3583 are passing in Substrait validation mode, which means that we'll need to add 3719 skipif DataFusionSubstraitRoundTrip (or similar) lines above each individual test that fails.

Some options here:

Add those 3719 skipif DataFusionSubstraitRoundTrip lines along with CI pipelines that run the sqllogictest in Substrait validation mode
Don't enforce Substrait validation immediately, and do some rounds solving the bugs that contribute the most errors in order to lower that number, and then do 1.
Don't enforce Substrait validation, and rely on contributors manually running them with the --substrait-round-trip flag

Don't enforce Substrait validation, and rely on contributors manually running them with the --substrait-round-trip flag

We could potentially start with this approach and then work towards getting the tests passing

The only worry I have is that the project will not be completed and then the code to do substrait validation would be abandoned (not used to test anything, but still in the repo). However, I think we an minimize this risk with some tickets tracking the eventual goal (turn on the --substrait-round-trip flag in CI)

The only worry I have is that the project will not be completed and then the code to do Substrait validation would be abandoned

At least at DataDog we have a big interest on getting this working as reliably as possible, as it's our only interface for feeding plans to DF, so our plan is to invest engineering time on this.

I understand (and share) the concern though, what comes to mind is to make this Substrait validation path as isolated as possible, so if things go south, deleting the unused code is trivial.

This is a cool idea! I wanted to see what the results look like, so I tried running cargo test --test sqllogictests -- --substrait-round-trip aggregate on your branch. It finds 9 errors in aggregate_skip_partial.slt and 136 in aggregate.slt.

I modified the sqllogic binary to print out all the errors, and counted the unique ones. It seems that lots of them are shared:

cargo test --test sqllogictests -- --substrait-round-trip aggregate > aggregate.log
cat aggregate.log|grep "query failed: "| cut -f 2- -d " " | sort | uniq -c

   7 query failed: DataFusion error: Error during planning: No table named 'tmp_table'
  20 query failed: DataFusion error: Internal error: Null cast is not valid.
   3 query failed: DataFusion error: Substrait error: Only literal types and aliases are supported in Virtual Tables, got: Cast
  16 query failed: DataFusion error: Substrait error: Only literal types and aliases are supported in Virtual Tables, got: ScalarFunction
   6 query failed: DataFusion error: This feature is not implemented: GroupingSet CUBE is not yet supported
  32 query failed: DataFusion error: This feature is not implemented: Producing a row from empty relation is unsupported
   2 query failed: DataFusion error: This feature is not implemented: Unsupported cast type: Dictionary(Int32, Utf8)
  15 query failed: DataFusion error: This feature is not implemented: Unsupported cast type: Dictionary(Int64, Int32)
   1 query failed: DataFusion error: This feature is not implemented: Unsupported cast type: Duration(Millisecond)
  12 query failed: DataFusion error: This feature is not implemented: Unsupported cast type: Duration(Second)
  11 query failed: DataFusion error: This feature is not implemented: Unsupported cast type: Time64(Nanosecond)

These seem like they would be fixable pretty easily.

There are also some more concerning ones, namely 20 cases where the conversion passes but the result seems to be "wrong".
Out of those, I can see couple patterns, there's some benign ones:

3x different temporary aliasing of columns failing EXPLAINs, but would result in correct result
5x slightly different plan otherwise, but would (?) result in correct result (extra Projection, swap Projection and Sort, duplicate Aggregate)

And some real issues:

9x wrong result due to not respecting IGNORE NULLS in array_agg/first_value/last_value
3x wrong result otherwise (range window, rollup)

It would be great to have these tests run somehow in CI, not failing it, but showing the failures somehow.. and then file issues for each type of failure, and then fix them 😄 Seems like it'd probably be possible to fix a big bunch of the errors by fixing just couple issues, so I think that'd speak for the option 2 from above:

Don't enforce Substrait validation immediately, and do some rounds solving the bugs that contribute the most errors in order to lower that number, and then do 1. (= add skipifs)

One takeaway could be to not do a roundtrip in EXPLAIN checks. In most cases the check will fail and it does not mean that the plan is incorrect.

so I think that'd speak for the option 2 from above

One of my main motivations for

Support filtering specific sqllogictests identified by line number #16029

is to be able to run the different failures in isolation and report them appropriately in tickets with the respective reproducer command.

What comes to mind is:

ship Support filtering specific sqllogictests identified by line number #16029 and then Add --substrait-round-trip option in sqllogictests gabotechs/datafusion#6
create a bunch of tickets with reproducer commands from the different kind of errors that we see
solve them until there's a fairly low amount of errors left
add skipif DataFusionSubstraitRoundtrip statements to each individual failing test
enforce cargo test --test sqllogictest -- --substrait-round-trip in the CI
hopefully solve all the remaining issues until there's no skipif statement left

What comes to mind is:

Sounds like a great plan to me!

Navigation Menu

Repository navigation

[substrait] Build basic test suite to validate produced Substrait plans #15069

Is your feature request related to a problem or challenge?

Describe the solution you'd like

Describe alternatives you've considered

Additional context

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[substrait] Build basic test suite to validate produced Substrait plans #15069

Description

Is your feature request related to a problem or challenge?

Describe the solution you'd like

Describe alternatives you've considered

Additional context

Activity

alamb commented on Mar 7, 2025

geoffreyclaude commented on Mar 21, 2025

geoffreyclaude commented on Apr 2, 2025

alamb commented on Apr 3, 2025

gabotechs commented on Apr 29, 2025

alamb commented on Apr 30, 2025

gabotechs commented on Apr 30, 2025

gabotechs commented on May 19, 2025

alamb commented on May 19, 2025

gabotechs commented on May 20, 2025

alamb commented on May 20, 2025

gabotechs commented on May 20, 2025

Blizzara commented on May 22, 2025

gabotechs commented on May 22, 2025

alamb commented on May 22, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions