Skip to content

Cover the configMatrix runner path with an end-to-end sqllogictest fixture - #24878

Open
bharadwaj-pendyala wants to merge 1 commit into
apache:mainfrom
bharadwaj-pendyala:test/config-matrix-slt-fixture
Open

Cover the configMatrix runner path with an end-to-end sqllogictest fixture#24878
bharadwaj-pendyala wants to merge 1 commit into
apache:mainfrom
bharadwaj-pendyala:test/config-matrix-slt-fixture

Conversation

@bharadwaj-pendyala

Copy link
Copy Markdown

Adds test_files/config_matrix.slt for the configMatrix runner path from #24493, requested by @kosiew in #24763.

It sweeps batch_size and time_zone; if overrides do not land, a combination reads 8192 and NULL. '2026-01-01T00:00:00Z'::timestamptz renders as 2026-01-01T05:30:00+05:30 or 2025-12-31T16:00:00-08:00 under the matching setting, so all four combinations expect true while asserting different results. CREATE TABLE cm_probe is not dropped, so a reused TestContext fails with DataFusion error: Execution error: Table 'cm_probe' already exists.

Test file only. run_each_configuration_runs_every_combination_past_failures covers per-combination dispatch. At base 3b6330084, config_matrix.slt passes and --lib is 35 passed, 0 failed. With apply_config_overrides short-circuited to Ok(()), all 4 combinations fail, 2 assertions each, including [configMatrix: datafusion.execution.batch_size=1, datafusion.execution.time_zone=+05:30]. time_zone replaced enable_ident_normalization=true,false because the latter's default is swept, so its neutered-overrides control passed.

@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Sep 2, 2026
@codecov-commenter

codecov-commenter commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.61%. Comparing base (35f58f5) to head (b5b46a3).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24878      +/-   ##
==========================================
- Coverage   81.61%   81.61%   -0.01%     
==========================================
  Files        1124     1124              
  Lines      411978   411978              
  Branches   411978   411978              
==========================================
- Hits       336236   336224      -12     
- Misses      55936    55944       +8     
- Partials    19806    19810       +4     

☔ 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.

@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.

@bharadwaj-pendyala,

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

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.

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.

The fixture now runs the predicate shape apache#24763 repaired, s = 'a' AND
'a' = s, across the matrix, and sweeps map_string_types_to_utf8view so
it runs over both a Utf8View and a Utf8 column. batch_size stays as the
check that a run applied overrides at all.

Requested by kosiew in review of apache#24878.
@bharadwaj-pendyala
bharadwaj-pendyala force-pushed the test/config-matrix-slt-fixture branch from 97d2c32 to b5b46a3 Compare September 5, 2026 03:09
@bharadwaj-pendyala

Copy link
Copy Markdown
Author

Added the s = 'a' AND 'a' = s matrix over map_string_types_to_utf8view=true,false; batch_size still checks that run overrides apply.

On current main at 35f58f53c, reverting datafusion/optimizer/src/simplify_expressions/simplify_predicates.rs to 20d1c5676^ doesn't reproduce the collapse: the targeted run has 3 tests (the two 24763-added tests are gone), SELECT count(*) FROM a WHERE s = 'a' AND 'a' = s; returns 1, and simplify_predicates.slt:247 passes with Filter: test_data.str_col = Utf8View("apple"). The join and ON cases, where 24763 noted canonicalization is skipped for Join, and the INT case also return the right answer. At max_passes=0, the unoptimized plan still shows CAST(Utf8("a") AS Utf8View) on both sides, so the input shape hasn't changed; simplify_predicates.slt isn't discriminating on main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants