Skip to content

Support config matrix for SLT tests - #24493

Open
comphead wants to merge 3 commits into
apache:mainfrom
comphead:slt_matrix
Open

Support config matrix for SLT tests#24493
comphead wants to merge 3 commits into
apache:mainfrom
comphead:slt_matrix

Conversation

@comphead

@comphead comphead commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Config knobs like datafusion.execution.parquet.coerce_int96 and coerce_int96_tz should produce the same observable result across values. Today that requires a separate .slt per combination. This PR
sweeps a cartesian product of config values from a single file.

What changes are included in this PR?

  • New # configMatrix: <key>=<v1>,<v2>[,...] directive. Repeat to nest dimensions; repeated keys merge value lists.
  • New datafusion/sqllogictest/src/config_matrix.rs - parser + expansion, exports parse_config_matrix_from_file, matrix_tag, ConfigMatrixCombination.
  • bin/sqllogictests.rs: run_test_file dispatches once per combo via run_test_file_once, which applies values through config_mut().options_mut().set(k, v) on a fresh SessionContext. Errors get a
    single [configMatrix: k=v, ...] suffix.
  • New test_files/parquet_int96_matrix.slt - 2×2 sweep over coerce_int96 and coerce_int96_tz.
  • Cookbook section added to datafusion/sqllogictest/README.md.

Are these changes tested?

  • 17 unit tests in config_matrix::tests cover parsing, dedup, merge, cartesian expansion, and error paths.
  • parquet_int96_matrix.slt exercises 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.

@comphead
comphead marked this pull request as draft August 19, 2026 17:02
@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Aug 19, 2026
@comphead comphead changed the title Slt matrix Support config matrix for SLT tests Aug 19, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.19856% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.24%. Comparing base (ebea069) to head (232c5e9).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/sqllogictest/bin/sqllogictests.rs 49.31% 34 Missing and 3 partials ⚠️
datafusion/sqllogictest/src/config_matrix.rs 98.03% 3 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@comphead
comphead marked this pull request as ready for review August 19, 2026 21:15
@comphead
comphead requested a review from kosiew August 19, 2026 21:16

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@comphead,

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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread datafusion/sqllogictest/README.md Outdated
- 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@github-actions github-actions Bot added the development-process Related to development process of DataFusion label Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

development-process Related to development process of DataFusion sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support config matrix for slt tests

3 participants