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

[amoeba]

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

[alamb]

Related ticket perhaps:

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

[geoffreyclaude]

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.)

[geoffreyclaude]

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.

[alamb]

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

[gabotechs]

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.

[alamb]

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

[gabotechs]

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.

[gabotechs]

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

[alamb]

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

1. Add a new CI job / phase of an existing job to run sqllogictests with substrait validation
2. 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)
3. File a ticket with the goal of "get all tests running in substrait validation mode"

[gabotechs]

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:

1. Add those 3719 skipif DataFusionSubstraitRoundTrip lines along with CI pipelines that run the sqllogictest in Substrait validation mode
2. 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.
3. Don't enforce Substrait validation, and rely on contributors manually running them with the --substrait-round-trip flag

[alamb]

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)