Remove sqllogictest fork swap from regenerate_sqlite_files.sh#21578
Open
RafaelHerrero wants to merge 1 commit intoapache:mainfrom
Open
Remove sqllogictest fork swap from regenerate_sqlite_files.sh#21578RafaelHerrero wants to merge 1 commit intoapache:mainfrom
RafaelHerrero wants to merge 1 commit intoapache:mainfrom
Conversation
The Omega359 fork features (valuewise result mode, valuesort, custom normalizer) were upstreamed to risinglightdb/sqllogictest-rs in v0.24.0 (PR risinglightdb/sqllogictest-rs#237). DataFusion already uses upstream v0.29.1 which includes everything, so the fork swap is no longer needed.
alamb
reviewed
Apr 15, 2026
Contributor
alamb
left a comment
There was a problem hiding this comment.
Thank you for this @RafaelHerrero . I tested running this script locally like
PG_URI=postgresql://postgres@localhost:5432/postgres nice ./datafusion/sqllogictest/regenerate_sqlite_files.sh
And it made a bunch of errors like this:
External error: task 948 panicked with message "called `Result::unwrap()` on an `Err` value: ParseError { kind: InvalidLine(\"onlyif mysql # use DIV operator for integer division\"), loc: Location { file: \"../../datafusion-testing/data/sqlite/random/select/slt_good_99.slt\", line: 60, upper: None } }"
Error: Execution("428 failures")
error: test failed, to rerun pass `-p datafusion-sqllogictest --test sqllogictests`
Caused by:
process didn't exit successfully: `/Users/andrewlamb/Software/datafusion2/target/release-nonlto/deps/sqllogictests-a999e2d90c5771c0 --include-sqlite --postgres-runner --complete` (exit status: 1)
Completion of sqlite test files failed!
Cleaning up source code changes and temporary files and directories
Do you have any idea what is going on?
maybe @Omega359 has some ideas as he was the original author of this script
Contributor
|
I thought I replied to this already but apparently hitting the 'comment' button was too much for me the other day :/ I think the fork does more than what was upstreamed though I'd have to diff the code at this point to know what exactly I changed. Ideally, yes I'd like to not have a fork for this but I haven't had the time nor desire to see what that would involve. |
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?
Related to #6543 and #21260
Rationale for this change
The
regenerate_sqlite_files.shscript swaps in a forked version ofsqllogictest-rs(Omega359/sqllogictest-rs v0.27.2) before running test completion. This fork added valuewise result mode, valuesort, and custom normalizer support.These features were upstreamed to
risinglightdb/sqllogictest-rsin v0.24.0 (PR #237). DataFusion already uses upstream v0.29.1 which includes everything, so the fork swap is no longer needed.What changes are included in this PR?
Cargo.tomldependency swap to the Omega359 fork (line 167)bin/sqllogictests.rsfile swap with the custom regenerate version (line 172)Are these changes tested?
The script has valid bash syntax. The change is purely removing dead code — the fork features are already available in the upstream version DataFusion uses.
Are there any user-facing changes?
No.