Support config matrix for SLT tests - #24493
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24493 +/- ##
==========================================
+ Coverage 81.22% 81.24% +0.01%
==========================================
Files 1113 1114 +1
Lines 392508 393016 +508
Branches 392508 393016 +508
==========================================
+ Hits 318833 319324 +491
- Misses 54916 54931 +15
- Partials 18759 18761 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The config matrix support looks useful and the overall implementation is nicely scoped.
I found one issue that I think needs to be addressed before merging. The Substrait round-trip path currently bypasses the config matrix handling, so a matrix-bearing SLT file can silently run only once in that mode. I also left one small documentation suggestion about where the matrix tag appears in failure output.
| options.substrait_round_trip, | ||
| ) { | ||
| (_, _, true) => { | ||
| run_test_file_substrait_round_trip( |
There was a problem hiding this comment.
It looks like --substrait-round-trip still dispatches directly to run_test_file_substrait_round_trip, so this path never parses or applies configMatrix combinations.
That means an .slt file with a matrix can silently run only once in this supported mode, even though the directive suggests the file will be exercised across all combinations. Could we either route this path through the same per-combination setup, or explicitly reject configMatrix when Substrait round-trip mode is used?
It would also be good to add a regression test that verifies each matrix combination is actually executed.
| - Repeat the directive to nest keys. Values are the cartesian product. | ||
| - Whitespace-trimmed and deduped; repeated keys merge value lists. | ||
| - Unknown key or invalid value fails fast, naming the file, key, and value. | ||
| - Test failures include `[configMatrix: k=v, ...]` in the `N errors in file …` banner. |
There was a problem hiding this comment.
Small documentation nit: this says the matrix tag is included in the N errors in file ... banner, but run_test_file_once currently appends the tag to the fully formatted error after the per-record errors.
Could we either move the tag into that banner or adjust the wording here to describe where it actually appears?
Which issue does this PR close?
Rationale for this change
Config knobs like
datafusion.execution.parquet.coerce_int96andcoerce_int96_tzshould produce the same observable result across values. Today that requires a separate.sltper combination. This PRsweeps a cartesian product of config values from a single file.
What changes are included in this PR?
# configMatrix: <key>=<v1>,<v2>[,...]directive. Repeat to nest dimensions; repeated keys merge value lists.datafusion/sqllogictest/src/config_matrix.rs- parser + expansion, exportsparse_config_matrix_from_file,matrix_tag,ConfigMatrixCombination.bin/sqllogictests.rs:run_test_filedispatches once per combo viarun_test_file_once, which applies values throughconfig_mut().options_mut().set(k, v)on a freshSessionContext. Errors get asingle
[configMatrix: k=v, ...]suffix.test_files/parquet_int96_matrix.slt- 2×2 sweep overcoerce_int96andcoerce_int96_tz.datafusion/sqllogictest/README.md.Are these changes tested?
config_matrix::testscover parsing, dedup, merge, cartesian expansion, and error paths.parquet_int96_matrix.sltexercises the runner end-to-end.Are there any user-facing changes?
Additive only. Files without a directive run byte-for-byte as before. Matrix-scoped failures include
[configMatrix: ...]in the banner.