Skip to content

Check sqllogictests for any dangling config settings (#17914)#20838

Merged
martin-g merged 7 commits intoapache:mainfrom
cj-zhukov:cj-zhukov/check-sqllogictests-for-any-dangling-config-settings-adv
Mar 13, 2026
Merged

Check sqllogictests for any dangling config settings (#17914)#20838
martin-g merged 7 commits intoapache:mainfrom
cj-zhukov:cj-zhukov/check-sqllogictests-for-any-dangling-config-settings-adv

Conversation

@cj-zhukov
Copy link
Copy Markdown
Contributor

@cj-zhukov cj-zhukov commented Mar 10, 2026

Which issue does this PR close?

Rationale for this change

In a previous PR #20474, I added a bash script that parsed the SLT files and checked whether any DataFusion configuration options were modified without being reset.

While that approach worked, it relied on external scripting and additional parsing logic. This PR introduces a simpler and more direct solution implemented in Rust.

At the end of each SLT test file execution, the current configuration is compared with the default configuration using a Drop implementation. If any configuration values were modified and not restored, a warning is printed.

This approach is easier to maintain and keeps the validation logic within the Rust codebase rather than relying on an external bash script.

What changes are included in this PR?

  • Capture the default DataFusion configuration when the SLT runner is initialized.
  • Implement Drop for the DataFusion SLT engine.
  • When an SLT file finishes executing, compare the current configuration with the default configuration.
  • If differences are detected, print a warning showing which configuration options were modified.

Are these changes tested?

This behavior is exercised by the existing SLT test suite. The configuration check runs automatically when each SLT file completes execution.

Are there any user-facing changes?

No. This change only affects internal SLT test infrastructure and does not modify any public APIs.

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Mar 10, 2026
@cj-zhukov
Copy link
Copy Markdown
Contributor Author

cj-zhukov commented Mar 10, 2026

@Jefffrey @martin-g Since you helped me with related issue, could you please take a look at this PR as well?

martin-g
martin-g reviewed Mar 10, 2026
View reviewed changes
cj-zhukov and others added 5 commits March 11, 2026 13:04
@cj-zhukov @martin-g
Update datafusion/sqllogictest/src/engines/datafusion_engine/runner.rs
151d638 
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
@cj-zhukov @martin-g
Update datafusion/sqllogictest/src/engines/datafusion_engine/runner.rs
b36dfe0 
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
@cj-zhukov @martin-g
Update datafusion/sqllogictest/src/engines/datafusion_engine/runner.rs
01bc068 
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
@cj-zhukov @martin-g
Update datafusion/sqllogictest/src/engines/datafusion_engine/runner.rs
0d9766f 
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
@cj-zhukov
use remove instead of get in drop method
1f4d17e
@cj-zhukov
Copy link
Copy Markdown
Contributor Author

cj-zhukov commented Mar 11, 2026

While testing this change I noticed that some configuration options appear as modified even though they are not explicitly set in the SLT file.

For example, in union.slt the test only sets:
set datafusion.execution.batch_size = 2;
However the final configuration also reports:
datafusion.execution.target_partitions: 8 -> 4

I am not entirely sure why this happens, but it looks like some configuration values may be modified internally during query planning or execution. As a result, the SLT file may leave the session configuration in a different state even though the change was not introduced by the test itself.

@martin-g
Copy link
Copy Markdown
Member

martin-g commented Mar 11, 2026

See

datafusion/datafusion/sqllogictest/src/test_context.rs

Line 83 in da05287

.with_target_partitions(4);

@cj-zhukov
Copy link
Copy Markdown
Contributor Author

cj-zhukov commented Mar 12, 2026

I updated the implementation to use the incoming ctx state as the baseline

@martin-g martin-g added this pull request to the merge queue Mar 13, 2026
Merged via the queue into apache:main with commit d2278a9 Mar 13, 2026
30 checks passed
@martin-g
Copy link
Copy Markdown
Member

martin-g commented Mar 13, 2026

Thank you, @cj-zhukov & @Jefffrey !

@cj-zhukov cj-zhukov deleted the cj-zhukov/check-sqllogictests-for-any-dangling-config-settings-adv branch March 13, 2026 11:51
@cj-zhukov cj-zhukov mentioned this pull request Mar 13, 2026
Closed
@2010YOUY01 2010YOUY01 mentioned this pull request Mar 23, 2026
Merged
de-bgunter pushed a commit to de-bgunter/datafusion that referenced this pull request Mar 24, 2026
@cj-zhukov @martin-g @de-bgunter
Check sqllogictests for any dangling config settings (apache#17914) (a…
423197e 
…pache#20838)

## Which issue does this PR close?

<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases.
You can link an issue to this PR using the GitHub syntax. For example
`Closes apache#123` indicates that this PR will close issue apache#123.
-->

- Closes #apache#17914.

## Rationale for this change
In a previous PR apache#20474, I
added a bash script that parsed the `SLT` files and checked whether any
DataFusion configuration options were modified without being reset.

While that approach worked, it relied on external scripting and
additional parsing logic. This PR introduces a simpler and more direct
solution implemented in Rust.

At the end of each `SLT` test file execution, the current configuration
is compared with the default configuration using a `Drop`
implementation. If any configuration values were modified and not
restored, a warning is printed.

This approach is easier to maintain and keeps the validation logic
within the Rust codebase rather than relying on an external bash script.

<!--
Why are you proposing this change? If this is already explained clearly
in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.
-->

## What changes are included in this PR?
- Capture the default DataFusion configuration when the `SLT` runner is
initialized.
- Implement `Drop` for the DataFusion SLT engine.
- When an `SLT` file finishes executing, compare the current
configuration with the default configuration.
- If differences are detected, print a warning showing which
configuration options were modified.

<!--
There is no need to duplicate the description in the issue here but it
is sometimes worth providing a summary of the individual changes in this
PR.
-->

## Are these changes tested?
This behavior is exercised by the existing `SLT` test suite. The
configuration check runs automatically when each `SLT` file completes
execution.

<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code

If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?
-->

## Are there any user-facing changes?
No. This change only affects internal `SLT` test infrastructure and does
not modify any public APIs.

<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
-->

<!--
If there are any breaking changes to public APIs, please add the `api
change` label.
-->

---------

Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Mar 24, 2026
@2010YOUY01
chore: Fix all sqllogictest dangling configs (#21108)
98defe6 
## Which issue does this PR close?

<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases.
You can link an issue to this PR using the GitHub syntax. For example
`Closes #123` indicates that this PR will close issue #123.
-->

Follow up to #20838

## Rationale for this change

<!--
Why are you proposing this change? If this is already explained clearly
in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.
-->

in #20838 it adds a way to check and warn about dangling config
settings, it has detected several existing violations but has not fixed
them:
```sh
yongting@Yongtings-MacBook-Pro-2 ~/C/datafusion (cleanup-slt)> git checkout main
Switched to branch 'main'
Your branch is up to date with 'upstream/main'.
yongting@Yongtings-MacBook-Pro-2 ~/C/datafusion (main=)> cargo test --profile=ci --test sqllogictests
   Compiling datafusion-sqllogictest v52.3.0 (/Users/yongting/Code/datafusion/datafusion/sqllogictest)
    Finished `ci` profile [unoptimized] target(s) in 2.09s
     Running bin/sqllogictests.rs (target/ci/deps/sqllogictests-7e2a8cc6115b158a)
Running with 14 test threads (available parallelism: 14)
SLT file aggregate_repartition.slt left modified configuration
  datafusion.execution.target_partitions: 4 -> 1
SLT file arrow_files.slt left modified configuration
  datafusion.sql_parser.map_string_types_to_utf8view: true -> false
SLT file datetime/current_date_timezone.slt left modified configuration
  datafusion.execution.time_zone: NULL -> +00:00
...
```

This PR does
1. Fix all the old violations
2. For future slt dangling configurations, it fails the test run instead
of only warn about it.

I tested to add a config statement at the end of one `slt` file, and run
the test, it runs and failed as expected
```sh
yongting@Yongtings-MacBook-Pro-2 ~/C/datafusion (cleanup-slt)> cargo test --profile=ci --test sqllogictests
   Compiling datafusion-sqllogictest v52.3.0 (/Users/yongting/Code/datafusion/datafusion/sqllogictest)
    Finished `ci` profile [unoptimized] target(s) in 2.28s
     Running bin/sqllogictests.rs (target/ci/deps/sqllogictests-7e2a8cc6115b158a)
Running with 14 test threads (available parallelism: 14)
Completed 416 test files in 5 seconds
External error: Other Error: SLT file sort_pushdown.slt left modified configuration
  datafusion.optimizer.max_passes: 3 -> 10
Error: Execution("1 failures")
error: test failed, to rerun pass `-p datafusion-sqllogictest --test sqllogictests`
```

## What changes are included in this PR?

<!--
There is no need to duplicate the description in the issue here but it
is sometimes worth providing a summary of the individual changes in this
PR.
-->

## Are these changes tested?

<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
4. Serve as another way to document the expected behavior of the code

If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?
-->
UT

## Are there any user-facing changes?

<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
-->

<!--
If there are any breaking changes to public APIs, please add the `api
change` label.
-->
no
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martin-g martin-g martin-g approved these changes

@Jefffrey Jefffrey Jefffrey approved these changes

Assignees

No one assigned

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cj-zhukov @martin-g @Jefffrey