Add --substrait-round-trip option in sqllogictests

[gabotechs]

Which issue does this PR close?

It probably does not fully closes it but it partially addresses:

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

Rationale for this change

Being able to execute the full sqllogictest suite adding a Substrait roundtrip. This means:

• Parsing the test statement
• Building a logical plan out of it
• Converting the logical plan to Substrait
• Converting the Substrait plan back to logical
• Execute it

What changes are included in this PR?

Adds a new DataFusionSubstraitRoundTrip sqllogictest runner implementation that will do a Substrait roundtrip where it makes sense, and a new --substrait-round-trip option to the sqllogictest CLI.

Example:

Runs the full aggregate.slt file doing a Substrait roundtrip on each individual statement

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

Runs an individual failing test reproducing one specific issue with the current Substrait conversions

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

This is still not enforced in the CI, and I would say the expectations are that it's just another tool to catch existing bugs in the current Substrait conversion code and better communicate them so that contributors have a clear path forward for contributing fixes.

Are these changes tested?

N/A

Are there any user-facing changes?

No

[gabotechs]

This one is good for a review! cc @alamb @Blizzara

[Blizzara]

Code looks fine to me but I'm not familiar with the sqllogictests code so will leave it to Andrew to review properly.

I really like this additional testing for Substrait, thanks!

[2010YOUY01]

Thanks! I suggest adding a CI test that exercises this option with a passing .slt file (or create a minimal one). Otherwise this LGTM.

[gabotechs]

I suggest adding a CI test that exercises this option with a passing .slt file

Something like this 1a05ba2?

[alamb]

This looks great @gabotechs -- thank you

I also tried it locally with tests I know fail and got an error as expected:

cargo test --test sqllogictests -- --substrait-round-trip agg

...

10. query failed: DataFusion error: This feature is not implemented: Producing a row from empty relation is unsupported
[SQL] with data as (
  select 1 as f,       4 as b
  union all
  select null as f,   99 as b
  union all
  select 2 as f,       5 as b
  union all
  select 98 as f,   null as b
  union all
  select 3 as f,       6 as b
  union all
  select null as f, null as b
)
select corr(f, b)
from data
at /Users/andrewlamb/Software/datafusion2/datafusion/sqllogictest/test_files/aggregate.slt:539

[alamb]

❤️

[alamb]

Thank you for the review @2010YOUY01

[findepi]

Don't ignore postgres_runner and complete arguments.
Unsupported arg combinations should be an error

[gabotechs]

I think clap should be taking care of that

datafusion/datafusion/sqllogictest/bin/sqllogictests.rs

Lines 636 to 642 in 76ff87b

	#[clap(
	long,
	conflicts_with = "complete",
	conflicts_with = "postgres_runner",
	help = "Before executing each query, convert its logical plan to Substrait and from Substrait back to its logical plan"
	)]
	substrait_round_trip: bool,

As suggested in #16183 (comment).

If you try to run incompatible commands you would get something like this:

error: the argument '--substrait-round-trip' cannot be used with '--complete'

[findepi]

good point and i haven't checked that

in the past i've observed some param combinations being ignored...
it's best if the clap does that (prints best error messages), but IMO the code further down should not rely on clap validation (think: args struct could come from some other source) and thus should re-validate.

anyway, that's a nit, since we have the clap validation