docs: fix the Substrait round-trip instructions in the sqllogictest README - #24855
Open
alexandrefimov wants to merge 2 commits into
Open
docs: fix the Substrait round-trip instructions in the sqllogictest README#24855alexandrefimov wants to merge 2 commits into
alexandrefimov wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-tripbut the round-trip path is behind the
substraitfeature, which is not indefault. Run as documented, every file fails withCannot run substrait round-trip: the 'substrait' feature is not enabled— 505 of them on today'smain, which reads as the mode being thoroughly broken rather than as a missing flag. Thextaskstep 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.ymlrunscargo xtask ci step test substrait, which filters tolimit.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,USINGjoin 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 overlimit.sltthrough thextaskstep, 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 thebinary.sltfilter runs three files. Rancargo xtask ci step test substraitto confirm the CI step passes overlimit.slt(it needs thetestingsubmodule; without it the file fails on missingaggregate_test_100data).ci/scripts/doc_prettier_check.shpasses.Are there any user-facing changes?
Documentation only.