Cover the configMatrix runner path with an end-to-end sqllogictest fixture - #24878
Cover the configMatrix runner path with an end-to-end sqllogictest fixture#24878bharadwaj-pendyala wants to merge 1 commit into
Conversation
config_matrix.rs unit-tests directive parsing and expansion, and apply_config_overrides is tested against a context directly. Nothing covered the runner wiring that builds a fresh context per combination and applies that combination's overrides before the engine snapshots config. Sweep batch_size and time_zone to values the default is not, so a combination whose overrides never landed reads 8192 and NULL and fails. The time zone assertion compares the rendered instant against the setting that produced it, so each combination asserts a different string while the expected value stays true.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24878 +/- ##
==========================================
- Coverage 81.63% 81.62% -0.01%
==========================================
Files 1123 1123
Lines 409673 409524 -149
Branches 409673 409524 -149
==========================================
- Hits 334417 334285 -132
+ Misses 55625 55599 -26
- Partials 19631 19640 +9 ☔ 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 adding the end-to-end configMatrix coverage. The fixture does a good job of checking that overrides are applied and that each matrix run gets a fresh TestContext.
I think there is one important gap relative to the original review request in #24763. That request was specifically about testing the #24763 changes with configMatrix, while the current fixture exercises batch_size and time_zone without running the predicate-simplification regression from #24763.
Could we adjust or extend the fixture so that the SQL executed by the matrix directly covers that regression? I left an example inline. The exact matrix settings in the example are illustrative, so it would be good to choose settings that actually exercise the relevant optimizer or execution paths.
Ideally, we should also verify that at least one matrix configuration fails against the pre-#24763 behavior and that all configurations pass with #24763 applied.
| # | ||
| # Matrix rules: no EXPLAIN, no in-file SET of a swept knob. | ||
|
|
||
| # configMatrix: datafusion.execution.batch_size=1,2 |
There was a problem hiding this comment.
Thanks for adding this fixture. Since the original request in #24763 was specifically to test those changes with configMatrix, could we make the SQL assertions exercise the predicate-simplification regression from #24763 directly? Right now batch_size and time_zone are useful for testing the configMatrix machinery, but they are not related to the behavior changed in #24763.
For example, we could run the affected predicate shape, such as s = 'a' AND 'a' = s, under a matrix of settings that meaningfully vary the relevant optimizer or execution path:
# configMatrix: datafusion.optimizer.max_passes=0,3
# configMatrix: datafusion.execution.parquet.pushdown_filters=true,false
statement ok
CREATE TABLE cm_predicate(s VARCHAR);
statement ok
INSERT INTO cm_predicate VALUES ('a'), ('b'), (NULL);
query I
SELECT count(*)
FROM cm_predicate
WHERE s = 'a' AND 'a' = s;
----
1
That would run the regression query across four configurations:
max_passes=0, pushdown_filters=true
max_passes=0, pushdown_filters=false
max_passes=3, pushdown_filters=true
max_passes=3, pushdown_filters=false
The settings above are just illustrative. I would prefer settings that we can confirm actually exercise the optimizer or execution paths involved in #24763.
It is also fine if every combination expects the same correct result. The useful regression property is that at least one relevant combination fails with the pre-#24763 behavior and all combinations pass with #24763 applied.
We can keep the existing undropped CREATE TABLE approach as well if we want to retain the fresh-context check. That would give us both end-to-end configMatrix coverage and direct regression coverage for the change that motivated the original review request.
Adds
test_files/config_matrix.sltfor theconfigMatrixrunner path from #24493, requested by @kosiew in #24763.It sweeps
batch_sizeandtime_zone; if overrides do not land, a combination reads 8192 and NULL.'2026-01-01T00:00:00Z'::timestamptzrenders as2026-01-01T05:30:00+05:30or2025-12-31T16:00:00-08:00under the matching setting, so all four combinations expecttruewhile asserting different results.CREATE TABLE cm_probeis not dropped, so a reusedTestContextfails withDataFusion error: Execution error: Table 'cm_probe' already exists.Test file only.
run_each_configuration_runs_every_combination_past_failurescovers per-combination dispatch. At base3b6330084,config_matrix.sltpasses and--libis 35 passed, 0 failed. Withapply_config_overridesshort-circuited toOk(()), all 4 combinations fail, 2 assertions each, including[configMatrix: datafusion.execution.batch_size=1, datafusion.execution.time_zone=+05:30].time_zonereplacedenable_ident_normalization=true,falsebecause the latter's default is swept, so its neutered-overrides control passed.