Skip to content

docs: fix the Substrait round-trip instructions in the sqllogictest README - #24855

Open
alexandrefimov wants to merge 2 commits into
apache:mainfrom
alexandrefimov:docs-sqllogictest-substrait-feature
Open

docs: fix the Substrait round-trip instructions in the sqllogictest README#24855
alexandrefimov wants to merge 2 commits into
apache:mainfrom
alexandrefimov:docs-sqllogictest-substrait-feature

Conversation

@alexandrefimov

@alexandrefimov alexandrefimov commented Sep 1, 2026

Copy link
Copy Markdown

Which issue does this PR close?

None — the documented command does not run and the paragraph around it is out of date, so this is a straight documentation fix.

Rationale for this change

The sqllogictest README documents the Substrait round-trip mode as

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

but the round-trip path is behind the substrait feature, which is not in default. Run as documented, every file fails with Cannot run substrait round-trip: the 'substrait' feature is not enabled — 505 of them on today's main, which reads as the mode being thoroughly broken rather than as a missing flag. The xtask step that CI uses gets this right (--features substrait); only the README does not.

The same paragraph says the mode "is not enforced in the CI, instead, it needs to be run manually". It is enforced, over one file: rust.yml runs cargo xtask ci step test substrait, which filters to limit.slt, with a comment saying the filter stays until most of #16248 is addressed. So a reader is told the mode has no CI coverage when it has a smoke test.

It also points at #16248 for the failures. That epic now has 16 of its 21 sub-issues closed, and several of the classes the mode reports most often today — Analyze, Unnest, USING join constraints, Distinct::On — have no sub-issue under it, so the sentence promises more than the link delivers.

What changes are included in this PR?

Both documented commands get --features substrait. The warning now says where the mode lives, that CI runs it over limit.slt through the xtask step, and that #16248 collects some of the failures rather than all of them.

What is the testing strategy for this PR?

Ran both documented forms on main: the full form runs the whole suite and reports its failures instead of stopping on the feature error, and the binary.slt filter runs three files. Ran cargo xtask ci step test substrait to confirm the CI step passes over limit.slt (it needs the testing submodule; without it the file fails on missing aggregate_test_100 data). ci/scripts/doc_prettier_check.sh passes.

Are there any user-facing changes?

Documentation only.

@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Sep 1, 2026
@alexandrefimov alexandrefimov changed the title docs: the Substrait round-trip test command needs the substrait feature docs: fix the Substrait round-trip instructions in the sqllogictest README Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant