You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "cj-zhukov (via GitHub)" <gi...@apache.org> on 2026/02/22 07:59:21 UTC
[PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
cj-zhukov opened a new pull request, #20474:
URL: https://github.com/apache/datafusion/pull/20474
## 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.
-->
- Closes #https://github.com/apache/datafusion/issues/17914.
## 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.
-->
## 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
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?
<!--
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.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#discussion_r2840661103
##########
ci/scripts/check_slt_configs.sh:
##########
@@ -0,0 +1,125 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Checks sqllogictest (.slt) files for dangling DataFusion configuration
+# settings.
+#
+# Any configuration set with:
+# set datafusion.<config> = true
+# must be reset in the same file with:
+# set datafusion.<config> = false
Review Comment:
What if the setting's default is `true` and an .slt test needed to set it to `false` but forgot to unset it ?
It would be better to use the `reset datafusion.***;` command instead
##########
ci/scripts/check_slt_configs.sh:
##########
@@ -0,0 +1,125 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Checks sqllogictest (.slt) files for dangling DataFusion configuration
+# settings.
+#
+# Any configuration set with:
+# set datafusion.<config> = true
+# must be reset in the same file with:
+# set datafusion.<config> = false
+#
+# This prevents configuration state from leaking into subsequent tests,
+# which can cause nondeterministic or hard-to-debug failures.
+#
+# Usage:
+# Check all .slt files under datafusion/sqllogictest/test_files:
+# ./check_slt_configs.sh
+#
+# Check a specific file (repo-relative or absolute path):
+# ./check_slt_configs.sh datafusion/sqllogictest/test_files/foo.slt
+# ./check_slt_configs.sh /absolute/path/to/foo.slt
+#
+# Exit codes:
+# 0 All checks passed
+# 1 One or more files contain configs set to true but never reset
+
+set -euo pipefail
+
+# Resolve repository root
+ROOT_DIR="$(git rev-parse --show-toplevel)"
+TARGET_DIR="${ROOT_DIR}/datafusion/sqllogictest/test_files"
+
+FAILED=0
+
+resolve_file() {
+ local input="$1"
+
+ # If absolute and exists
+ if [[ -f "$input" ]]; then
+ realpath "$input"
+ return
+ fi
+
+ # If relative to repo root
+ if [[ -f "${ROOT_DIR}/${input}" ]]; then
+ realpath "${ROOT_DIR}/${input}"
+ return
+ fi
+
+ echo ""
+}
+
+# If file argument is passed, check only that file
+if [[ $# -eq 1 ]]; then
+ RESOLVED_FILE="$(resolve_file "$1")"
+
+ if [[ -z "$RESOLVED_FILE" ]]; then
+ echo "❌ File does not exist: $1"
+ exit 1
+ fi
+
+ FILES="$RESOLVED_FILE"
+else
+ FILES=$(find "$TARGET_DIR" -type f -name "*.slt")
+fi
+
+# Iterate safely even if filenames contain spaces
+while IFS= read -r file; do
+ echo "Checking file: $file"
+
+ if [[ ! -f "$file" ]]; then
+ echo " ❌ File not found: $file"
+ FAILED=1
+ continue
+ fi
+
+ matches=$(grep -En \
+ 'set[[:space:]]+datafusion\.[a-zA-Z0-9_.]+[[:space:]]*=[[:space:]]*true' \
+ "$file" || true)
+
+ [[ -z "$matches" ]] && continue
+
+ # Process each match line-by-line
+ while IFS= read -r match; do
+ line_number=$(echo "$match" | cut -d: -f1)
+ line_content=$(echo "$match" | cut -d: -f2-)
Review Comment:
nit: You could avoid starting a new process by using Bash-isms like:
```
line_number=${match%%:*}
line_content=${match#*:}
```
##########
ci/scripts/check_slt_configs.sh:
##########
@@ -0,0 +1,125 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Checks sqllogictest (.slt) files for dangling DataFusion configuration
+# settings.
+#
+# Any configuration set with:
+# set datafusion.<config> = true
+# must be reset in the same file with:
+# set datafusion.<config> = false
+#
+# This prevents configuration state from leaking into subsequent tests,
+# which can cause nondeterministic or hard-to-debug failures.
+#
+# Usage:
+# Check all .slt files under datafusion/sqllogictest/test_files:
+# ./check_slt_configs.sh
+#
+# Check a specific file (repo-relative or absolute path):
+# ./check_slt_configs.sh datafusion/sqllogictest/test_files/foo.slt
+# ./check_slt_configs.sh /absolute/path/to/foo.slt
+#
+# Exit codes:
+# 0 All checks passed
+# 1 One or more files contain configs set to true but never reset
+
+set -euo pipefail
+
+# Resolve repository root
+ROOT_DIR="$(git rev-parse --show-toplevel)"
+TARGET_DIR="${ROOT_DIR}/datafusion/sqllogictest/test_files"
+
+FAILED=0
+
+resolve_file() {
+ local input="$1"
+
+ # If absolute and exists
+ if [[ -f "$input" ]]; then
+ realpath "$input"
+ return
+ fi
+
+ # If relative to repo root
+ if [[ -f "${ROOT_DIR}/${input}" ]]; then
+ realpath "${ROOT_DIR}/${input}"
+ return
+ fi
+
+ echo ""
Review Comment:
This looks strange.
Why not log the error from line 74 here and exit ?
##########
.github/workflows/rust.yml:
##########
@@ -742,7 +742,33 @@ jobs:
- name: Run examples docs check script
run: |
bash ci/scripts/check_examples_docs.sh
+
+ # This job ensures that sqllogictest (.slt) files do not leave
+ # DataFusion configuration options enabled (e.g. `set ... = true`)
+ # without resetting them back to their default state.
+ #
+ # It runs ci/scripts/check_slt_configs.sh and fails if any
+ # dangling configuration settings are detected.
+ sqllogictest-config-check:
+ name: check sqllogictest configs are properly reset
+ needs: linux-build-lib
+ runs-on: ubuntu-latest
+ container:
+ image: amd64/rust
Review Comment:
Any reason to use a container ?
The CI job just runs a Bash shell script
##########
ci/scripts/check_slt_configs.sh:
##########
@@ -0,0 +1,125 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Checks sqllogictest (.slt) files for dangling DataFusion configuration
+# settings.
+#
+# Any configuration set with:
+# set datafusion.<config> = true
+# must be reset in the same file with:
+# set datafusion.<config> = false
+#
+# This prevents configuration state from leaking into subsequent tests,
+# which can cause nondeterministic or hard-to-debug failures.
+#
+# Usage:
+# Check all .slt files under datafusion/sqllogictest/test_files:
+# ./check_slt_configs.sh
+#
+# Check a specific file (repo-relative or absolute path):
+# ./check_slt_configs.sh datafusion/sqllogictest/test_files/foo.slt
+# ./check_slt_configs.sh /absolute/path/to/foo.slt
+#
+# Exit codes:
+# 0 All checks passed
+# 1 One or more files contain configs set to true but never reset
+
+set -euo pipefail
+
+# Resolve repository root
+ROOT_DIR="$(git rev-parse --show-toplevel)"
+TARGET_DIR="${ROOT_DIR}/datafusion/sqllogictest/test_files"
+
+FAILED=0
+
+resolve_file() {
+ local input="$1"
+
+ # If absolute and exists
+ if [[ -f "$input" ]]; then
+ realpath "$input"
+ return
+ fi
+
+ # If relative to repo root
+ if [[ -f "${ROOT_DIR}/${input}" ]]; then
+ realpath "${ROOT_DIR}/${input}"
+ return
+ fi
+
+ echo ""
+}
+
+# If file argument is passed, check only that file
+if [[ $# -eq 1 ]]; then
+ RESOLVED_FILE="$(resolve_file "$1")"
+
+ if [[ -z "$RESOLVED_FILE" ]]; then
+ echo "❌ File does not exist: $1"
+ exit 1
+ fi
+
+ FILES="$RESOLVED_FILE"
+else
+ FILES=$(find "$TARGET_DIR" -type f -name "*.slt")
+fi
+
+# Iterate safely even if filenames contain spaces
+while IFS= read -r file; do
+ echo "Checking file: $file"
+
+ if [[ ! -f "$file" ]]; then
+ echo " ❌ File not found: $file"
+ FAILED=1
+ continue
+ fi
+
+ matches=$(grep -En \
+ 'set[[:space:]]+datafusion\.[a-zA-Z0-9_.]+[[:space:]]*=[[:space:]]*true' \
+ "$file" || true)
+
+ [[ -z "$matches" ]] && continue
+
+ # Process each match line-by-line
+ while IFS= read -r match; do
+ line_number=$(echo "$match" | cut -d: -f1)
+ line_content=$(echo "$match" | cut -d: -f2-)
+
+ # Extract config name
+ config=$(echo "$line_content" \
+ | sed -E 's/set[[:space:]]+(datafusion\.[a-zA-Z0-9_.]+).*/\1/')
+
+ # Check if reset exists anywhere in file
+ if ! grep -Eq \
+ "set[[:space:]]+$config[[:space:]]*=[[:space:]]*false" \
Review Comment:
```suggestion
"set[[:space:]]+${config}[[:space:]]*=[[:space:]]*false" \
```
##########
datafusion/sqllogictest/test_files/push_down_filter.slt:
##########
@@ -743,3 +744,18 @@ drop table t1;
statement ok
drop table t2;
+
+statement ok
+set datafusion.execution.parquet.pushdown_filters = false;
+
+statement ok
+set datafusion.execution.collect_statistics = false;
+
+statement ok
+set datafusion.execution.parquet.pushdown_filters = false;
Review Comment:
duplicate of line 749
##########
datafusion/sqllogictest/test_files/information_schema_multiple_catalogs.slt:
##########
Review Comment:
The catalog and the schema below are never reset.
This is an example of non-boolean settings.
##########
ci/scripts/check_slt_configs.sh:
##########
@@ -0,0 +1,125 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Checks sqllogictest (.slt) files for dangling DataFusion configuration
+# settings.
+#
+# Any configuration set with:
+# set datafusion.<config> = true
+# must be reset in the same file with:
+# set datafusion.<config> = false
+#
+# This prevents configuration state from leaking into subsequent tests,
+# which can cause nondeterministic or hard-to-debug failures.
+#
+# Usage:
+# Check all .slt files under datafusion/sqllogictest/test_files:
+# ./check_slt_configs.sh
+#
+# Check a specific file (repo-relative or absolute path):
+# ./check_slt_configs.sh datafusion/sqllogictest/test_files/foo.slt
+# ./check_slt_configs.sh /absolute/path/to/foo.slt
+#
+# Exit codes:
+# 0 All checks passed
+# 1 One or more files contain configs set to true but never reset
+
+set -euo pipefail
+
+# Resolve repository root
+ROOT_DIR="$(git rev-parse --show-toplevel)"
+TARGET_DIR="${ROOT_DIR}/datafusion/sqllogictest/test_files"
+
+FAILED=0
+
+resolve_file() {
+ local input="$1"
+
+ # If absolute and exists
+ if [[ -f "$input" ]]; then
+ realpath "$input"
+ return
+ fi
+
+ # If relative to repo root
+ if [[ -f "${ROOT_DIR}/${input}" ]]; then
+ realpath "${ROOT_DIR}/${input}"
+ return
+ fi
+
+ echo ""
+}
+
+# If file argument is passed, check only that file
+if [[ $# -eq 1 ]]; then
+ RESOLVED_FILE="$(resolve_file "$1")"
+
+ if [[ -z "$RESOLVED_FILE" ]]; then
+ echo "❌ File does not exist: $1"
+ exit 1
+ fi
+
+ FILES="$RESOLVED_FILE"
+else
+ FILES=$(find "$TARGET_DIR" -type f -name "*.slt")
+fi
+
+# Iterate safely even if filenames contain spaces
+while IFS= read -r file; do
+ echo "Checking file: $file"
+
+ if [[ ! -f "$file" ]]; then
+ echo " ❌ File not found: $file"
+ FAILED=1
+ continue
+ fi
+
+ matches=$(grep -En \
+ 'set[[:space:]]+datafusion\.[a-zA-Z0-9_.]+[[:space:]]*=[[:space:]]*true' \
+ "$file" || true)
+
+ [[ -z "$matches" ]] && continue
+
+ # Process each match line-by-line
+ while IFS= read -r match; do
+ line_number=$(echo "$match" | cut -d: -f1)
+ line_content=$(echo "$match" | cut -d: -f2-)
+
+ # Extract config name
+ config=$(echo "$line_content" \
+ | sed -E 's/set[[:space:]]+(datafusion\.[a-zA-Z0-9_.]+).*/\1/')
+
+ # Check if reset exists anywhere in file
+ if ! grep -Eq \
+ "set[[:space:]]+$config[[:space:]]*=[[:space:]]*false" \
Review Comment:
This should also support the `reset datafusion.***` command
##########
.github/workflows/rust.yml:
##########
@@ -742,7 +742,33 @@ jobs:
- name: Run examples docs check script
run: |
bash ci/scripts/check_examples_docs.sh
+
+ # This job ensures that sqllogictest (.slt) files do not leave
+ # DataFusion configuration options enabled (e.g. `set ... = true`)
+ # without resetting them back to their default state.
+ #
+ # It runs ci/scripts/check_slt_configs.sh and fails if any
+ # dangling configuration settings are detected.
+ sqllogictest-config-check:
+ name: check sqllogictest configs are properly reset
+ needs: linux-build-lib
Review Comment:
Is there any reason to wait for the Rust build ?
IMO the checks could be done earlier.
##########
datafusion/sqllogictest/test_files/set_variable.slt:
##########
@@ -447,3 +447,6 @@ datafusion.runtime.max_temp_directory_size
datafusion.runtime.memory_limit
datafusion.runtime.metadata_cache_limit
datafusion.runtime.temp_directory
+
+statement ok
+set datafusion.catalog.information_schema = false
Review Comment:
```suggestion
set datafusion.catalog.information_schema = false;
```
##########
ci/scripts/check_slt_configs.sh:
##########
@@ -0,0 +1,125 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Checks sqllogictest (.slt) files for dangling DataFusion configuration
+# settings.
+#
+# Any configuration set with:
+# set datafusion.<config> = true
+# must be reset in the same file with:
+# set datafusion.<config> = false
+#
+# This prevents configuration state from leaking into subsequent tests,
+# which can cause nondeterministic or hard-to-debug failures.
+#
+# Usage:
+# Check all .slt files under datafusion/sqllogictest/test_files:
+# ./check_slt_configs.sh
+#
+# Check a specific file (repo-relative or absolute path):
+# ./check_slt_configs.sh datafusion/sqllogictest/test_files/foo.slt
+# ./check_slt_configs.sh /absolute/path/to/foo.slt
+#
+# Exit codes:
+# 0 All checks passed
+# 1 One or more files contain configs set to true but never reset
+
+set -euo pipefail
+
+# Resolve repository root
+ROOT_DIR="$(git rev-parse --show-toplevel)"
+TARGET_DIR="${ROOT_DIR}/datafusion/sqllogictest/test_files"
+
+FAILED=0
+
+resolve_file() {
+ local input="$1"
+
+ # If absolute and exists
+ if [[ -f "$input" ]]; then
+ realpath "$input"
+ return
+ fi
+
+ # If relative to repo root
+ if [[ -f "${ROOT_DIR}/${input}" ]]; then
+ realpath "${ROOT_DIR}/${input}"
+ return
+ fi
+
+ echo ""
+}
+
+# If file argument is passed, check only that file
+if [[ $# -eq 1 ]]; then
+ RESOLVED_FILE="$(resolve_file "$1")"
+
+ if [[ -z "$RESOLVED_FILE" ]]; then
+ echo "❌ File does not exist: $1"
+ exit 1
+ fi
+
+ FILES="$RESOLVED_FILE"
+else
+ FILES=$(find "$TARGET_DIR" -type f -name "*.slt")
+fi
+
+# Iterate safely even if filenames contain spaces
+while IFS= read -r file; do
+ echo "Checking file: $file"
+
+ if [[ ! -f "$file" ]]; then
+ echo " ❌ File not found: $file"
+ FAILED=1
+ continue
+ fi
+
+ matches=$(grep -En \
+ 'set[[:space:]]+datafusion\.[a-zA-Z0-9_.]+[[:space:]]*=[[:space:]]*true' \
Review Comment:
`set` could also be `SET`
See https://github.com/cj-zhukov/datafusion/blob/fe921dca542e84a48bc02e3c409ce7ac45686283/datafusion/sqllogictest/test_files/set_variable.slt#L23 for such example
##########
datafusion/sqllogictest/test_files/type_coercion.slt:
##########
@@ -281,3 +281,6 @@ SELECT true FROM t0 WHERE (REGEXP_MATCH(t0.v1, t0.v1)) NOT ILIKE [];
statement ok
DROP TABLE t0;
+
+statement ok
+set datafusion.explain.logical_plan_only = false;
Review Comment:
This is already done at https://github.com/apache/datafusion/pull/20474/changes#diff-7f1b4e520d77b90cae272fc9d03f3c397d1ab10e47de434ff9f8058ffd7cc083R249
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#issuecomment-3957162614
> Is there a way to do this check in the SLT runner test code itself? e.g. after it runs an SLT file it checks if the config state is dirty and reports it via a warning in test output? I'm not particularly sure of adding more and more bash scripts like this
Thank you - that makes sense. I agree that enforcing this at the SLT runner level would be a cleaner and more robust solution than adding additional Bash-based checks in CI.
I initially considered implementing this directly in Rust, but the shell script felt like a simpler and less invasive first step. That said, I understand the concern about accumulating more CI scripting logic, and I agree that handling this inside the SLT runner is architecturally the better approach.
We can mark this PR as draft (without closing the root issue). I'm going to work on a follow-up PR that implements the check directly in the SLT runner - e.g., verifying after each SLT file execution that the configuration state matches the default and reporting it in the test output. That should give us a more complete and future-proof solution.
Let me know if that sounds like a good direction, and I’ll proceed accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#discussion_r2844721852
##########
datafusion/sqllogictest/test_files/push_down_filter.slt:
##########
@@ -743,3 +744,18 @@ drop table t1;
statement ok
drop table t2;
+
+statement ok
+set datafusion.execution.parquet.pushdown_filters = false;
+
+statement ok
+set datafusion.execution.collect_statistics = false;
+
+statement ok
+set datafusion.execution.parquet.pushdown_filters = false;
Review Comment:
Thank you for catching this - I missed that duplicate configuration in the `push_down_filter.slt`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#issuecomment-3940604631
@Jefffrey Since you originally opened this issue, could you please take a look at this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#issuecomment-3957753864
That sounds good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#discussion_r2844801700
##########
datafusion/sqllogictest/test_files/set_variable.slt:
##########
@@ -447,3 +447,6 @@ datafusion.runtime.max_temp_directory_size
datafusion.runtime.memory_limit
datafusion.runtime.metadata_cache_limit
datafusion.runtime.temp_directory
+
+statement ok
+set datafusion.catalog.information_schema = false
Review Comment:
I noticed that other `SET` statements in this file do not use semicolons.
Should we standardize on including `;`, or keep the existing style for consistency?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov closed pull request #20474: Check sqllogictests for any dangling config settings(#17914)
URL: https://github.com/apache/datafusion/pull/20474
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#discussion_r2844810704
##########
ci/scripts/check_slt_configs.sh:
##########
@@ -0,0 +1,125 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Checks sqllogictest (.slt) files for dangling DataFusion configuration
+# settings.
+#
+# Any configuration set with:
+# set datafusion.<config> = true
+# must be reset in the same file with:
+# set datafusion.<config> = false
+#
+# This prevents configuration state from leaking into subsequent tests,
+# which can cause nondeterministic or hard-to-debug failures.
+#
+# Usage:
+# Check all .slt files under datafusion/sqllogictest/test_files:
+# ./check_slt_configs.sh
+#
+# Check a specific file (repo-relative or absolute path):
+# ./check_slt_configs.sh datafusion/sqllogictest/test_files/foo.slt
+# ./check_slt_configs.sh /absolute/path/to/foo.slt
+#
+# Exit codes:
+# 0 All checks passed
+# 1 One or more files contain configs set to true but never reset
+
+set -euo pipefail
+
+# Resolve repository root
+ROOT_DIR="$(git rev-parse --show-toplevel)"
+TARGET_DIR="${ROOT_DIR}/datafusion/sqllogictest/test_files"
+
+FAILED=0
+
+resolve_file() {
+ local input="$1"
+
+ # If absolute and exists
+ if [[ -f "$input" ]]; then
+ realpath "$input"
+ return
+ fi
+
+ # If relative to repo root
+ if [[ -f "${ROOT_DIR}/${input}" ]]; then
+ realpath "${ROOT_DIR}/${input}"
+ return
+ fi
+
+ echo ""
+}
+
+# If file argument is passed, check only that file
+if [[ $# -eq 1 ]]; then
+ RESOLVED_FILE="$(resolve_file "$1")"
+
+ if [[ -z "$RESOLVED_FILE" ]]; then
+ echo "❌ File does not exist: $1"
+ exit 1
+ fi
+
+ FILES="$RESOLVED_FILE"
+else
+ FILES=$(find "$TARGET_DIR" -type f -name "*.slt")
+fi
+
+# Iterate safely even if filenames contain spaces
+while IFS= read -r file; do
+ echo "Checking file: $file"
+
+ if [[ ! -f "$file" ]]; then
+ echo " ❌ File not found: $file"
+ FAILED=1
+ continue
+ fi
+
+ matches=$(grep -En \
+ 'set[[:space:]]+datafusion\.[a-zA-Z0-9_.]+[[:space:]]*=[[:space:]]*true' \
+ "$file" || true)
+
+ [[ -z "$matches" ]] && continue
+
+ # Process each match line-by-line
+ while IFS= read -r match; do
+ line_number=$(echo "$match" | cut -d: -f1)
+ line_content=$(echo "$match" | cut -d: -f2-)
+
+ # Extract config name
+ config=$(echo "$line_content" \
+ | sed -E 's/set[[:space:]]+(datafusion\.[a-zA-Z0-9_.]+).*/\1/')
+
+ # Check if reset exists anywhere in file
+ if ! grep -Eq \
+ "set[[:space:]]+$config[[:space:]]*=[[:space:]]*false" \
Review Comment:
Agree with you - I''m going to work on it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#issuecomment-3940458481
### High-Level Overview
This PR introduces a validation script to prevent dangling configuration settings in sqllogictest (`.slt`) files.
This PR Adds:
- A new shell script `ci/scripts/check_slt_configs.sh` that scans all `.slt` files and detects DataFusion configuration options that are set to: `set datafusion.<config> = true` but are never reset back to: `set datafusion.<config> = false`
- Updates to existing `.slt` files where dangling boolean configs were found.
All identified configs have been explicitly reset to false to ensure files restore the default session state.
- A new CI job: `sqllogictest-config-check` that runs the validation script and fails the workflow if any abandoned boolean configuration is detected.
Current Limitations:
- Only checks boolean flags set to `true` and id does not validate non-boolean configuration changes (e.g. default_catalog, default_schema, etc.)
- Does not respect ordering: It only verifies that a matching = false exists somewhere in the file and It does not ensure the reset occurs after the corresponding = true.
- Does not validate toggle correctness: it does not verify correct pairing, nesting, or the number of enable/disable occurrence
Follow-Up Improvements:
- Parse (`.slt`) files properly instead of relying on regex/grep.
- Track configuration state transitions in order.
- Validate correct pairing and restoration semantics.
- Support non-boolean configuration values.
This could be implemented in Rust using a proper parser (e.g., `nom`), similar to what is already used in `datafusion-examples`.
I’m happy to work on a follow-up PR to evolve this validation into a more robust and structured implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#discussion_r2844659465
##########
ci/scripts/check_slt_configs.sh:
##########
@@ -0,0 +1,125 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Checks sqllogictest (.slt) files for dangling DataFusion configuration
+# settings.
+#
+# Any configuration set with:
+# set datafusion.<config> = true
+# must be reset in the same file with:
+# set datafusion.<config> = false
+#
+# This prevents configuration state from leaking into subsequent tests,
+# which can cause nondeterministic or hard-to-debug failures.
+#
+# Usage:
+# Check all .slt files under datafusion/sqllogictest/test_files:
+# ./check_slt_configs.sh
+#
+# Check a specific file (repo-relative or absolute path):
+# ./check_slt_configs.sh datafusion/sqllogictest/test_files/foo.slt
+# ./check_slt_configs.sh /absolute/path/to/foo.slt
+#
+# Exit codes:
+# 0 All checks passed
+# 1 One or more files contain configs set to true but never reset
+
+set -euo pipefail
+
+# Resolve repository root
+ROOT_DIR="$(git rev-parse --show-toplevel)"
+TARGET_DIR="${ROOT_DIR}/datafusion/sqllogictest/test_files"
+
+FAILED=0
+
+resolve_file() {
+ local input="$1"
+
+ # If absolute and exists
+ if [[ -f "$input" ]]; then
+ realpath "$input"
+ return
+ fi
+
+ # If relative to repo root
+ if [[ -f "${ROOT_DIR}/${input}" ]]; then
+ realpath "${ROOT_DIR}/${input}"
+ return
+ fi
+
+ echo ""
Review Comment:
I agree with you - let's do it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#discussion_r2847364598
##########
datafusion/sqllogictest/test_files/information_schema_multiple_catalogs.slt:
##########
Review Comment:
Thank you for pointing this out. You’re right - this is currently a limitation. The script only checks boolean settings enabled via `= true` and does not detect non-boolean configuration changes.
At this stage, I’d prefer to keep the script simple rather than significantly increasing its complexity in Bash. If we decide to expand the scope to cover all `SET datafusion.*` mutations (including non-boolean values), I think it would be better to address that in a follow-up by implementing a more robust solution in Rust (e.g., using `nom`). That would allow us to properly track configuration changes and ensure they are restored in a more reliable way. I'm ready to work on this.
Happy to hear your thoughts on whether we should keep this PR focused or broaden its scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#issuecomment-4054574425
I'm closing this PR as all the work was completed and merged in https://github.com/apache/datafusion/pull/20838
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#discussion_r2844853644
##########
.github/workflows/rust.yml:
##########
@@ -742,7 +742,33 @@ jobs:
- name: Run examples docs check script
run: |
bash ci/scripts/check_examples_docs.sh
+
+ # This job ensures that sqllogictest (.slt) files do not leave
+ # DataFusion configuration options enabled (e.g. `set ... = true`)
+ # without resetting them back to their default state.
+ #
+ # It runs ci/scripts/check_slt_configs.sh and fails if any
+ # dangling configuration settings are detected.
+ sqllogictest-config-check:
+ name: check sqllogictest configs are properly reset
+ needs: linux-build-lib
+ runs-on: ubuntu-latest
+ container:
+ image: amd64/rust
Review Comment:
That's correct - no reason for a container. I'll run it directly on `ubuntu-latest` instead of using the Rust container so it runs earlier and fails fast.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#discussion_r2844850601
##########
.github/workflows/rust.yml:
##########
@@ -742,7 +742,33 @@ jobs:
- name: Run examples docs check script
run: |
bash ci/scripts/check_examples_docs.sh
+
+ # This job ensures that sqllogictest (.slt) files do not leave
+ # DataFusion configuration options enabled (e.g. `set ... = true`)
+ # without resetting them back to their default state.
+ #
+ # It runs ci/scripts/check_slt_configs.sh and fails if any
+ # dangling configuration settings are detected.
+ sqllogictest-config-check:
+ name: check sqllogictest configs are properly reset
+ needs: linux-build-lib
Review Comment:
Good point - this job does not depend on the Rust build or any compiled artifacts, and it only runs a Bash script. I’ll remove the needs: `linux-build-lib` dependency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#discussion_r2849392057
##########
datafusion/sqllogictest/test_files/information_schema_multiple_catalogs.slt:
##########
Review Comment:
Agreed! Keep it simple for the first version!
Maybe add a comment/TODO to the new Bash script about this use case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#discussion_r2844651849
##########
ci/scripts/check_slt_configs.sh:
##########
@@ -0,0 +1,125 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Checks sqllogictest (.slt) files for dangling DataFusion configuration
+# settings.
+#
+# Any configuration set with:
+# set datafusion.<config> = true
+# must be reset in the same file with:
+# set datafusion.<config> = false
Review Comment:
Good point - using `RESET` is semantically more correct since it restores the default value rather than assuming it is `false`. I'm going to update the script to accept `RESET datafusion.<config>` as a valid restoration mechanism.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#discussion_r2844711505
##########
ci/scripts/check_slt_configs.sh:
##########
@@ -0,0 +1,125 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Checks sqllogictest (.slt) files for dangling DataFusion configuration
+# settings.
+#
+# Any configuration set with:
+# set datafusion.<config> = true
+# must be reset in the same file with:
+# set datafusion.<config> = false
+#
+# This prevents configuration state from leaking into subsequent tests,
+# which can cause nondeterministic or hard-to-debug failures.
+#
+# Usage:
+# Check all .slt files under datafusion/sqllogictest/test_files:
+# ./check_slt_configs.sh
+#
+# Check a specific file (repo-relative or absolute path):
+# ./check_slt_configs.sh datafusion/sqllogictest/test_files/foo.slt
+# ./check_slt_configs.sh /absolute/path/to/foo.slt
+#
+# Exit codes:
+# 0 All checks passed
+# 1 One or more files contain configs set to true but never reset
+
+set -euo pipefail
+
+# Resolve repository root
+ROOT_DIR="$(git rev-parse --show-toplevel)"
+TARGET_DIR="${ROOT_DIR}/datafusion/sqllogictest/test_files"
+
+FAILED=0
+
+resolve_file() {
+ local input="$1"
+
+ # If absolute and exists
+ if [[ -f "$input" ]]; then
+ realpath "$input"
+ return
+ fi
+
+ # If relative to repo root
+ if [[ -f "${ROOT_DIR}/${input}" ]]; then
+ realpath "${ROOT_DIR}/${input}"
+ return
+ fi
+
+ echo ""
+}
+
+# If file argument is passed, check only that file
+if [[ $# -eq 1 ]]; then
+ RESOLVED_FILE="$(resolve_file "$1")"
+
+ if [[ -z "$RESOLVED_FILE" ]]; then
+ echo "❌ File does not exist: $1"
+ exit 1
+ fi
+
+ FILES="$RESOLVED_FILE"
+else
+ FILES=$(find "$TARGET_DIR" -type f -name "*.slt")
+fi
+
+# Iterate safely even if filenames contain spaces
+while IFS= read -r file; do
+ echo "Checking file: $file"
+
+ if [[ ! -f "$file" ]]; then
+ echo " ❌ File not found: $file"
+ FAILED=1
+ continue
+ fi
+
+ matches=$(grep -En \
+ 'set[[:space:]]+datafusion\.[a-zA-Z0-9_.]+[[:space:]]*=[[:space:]]*true' \
+ "$file" || true)
+
+ [[ -z "$matches" ]] && continue
+
+ # Process each match line-by-line
+ while IFS= read -r match; do
+ line_number=$(echo "$match" | cut -d: -f1)
+ line_content=$(echo "$match" | cut -d: -f2-)
Review Comment:
Good suggestion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Check sqllogictests for any dangling config settings(#17914) [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #20474:
URL: https://github.com/apache/datafusion/pull/20474#discussion_r2847303796
##########
ci/scripts/check_slt_configs.sh:
##########
@@ -0,0 +1,125 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Checks sqllogictest (.slt) files for dangling DataFusion configuration
+# settings.
+#
+# Any configuration set with:
+# set datafusion.<config> = true
+# must be reset in the same file with:
+# set datafusion.<config> = false
+#
+# This prevents configuration state from leaking into subsequent tests,
+# which can cause nondeterministic or hard-to-debug failures.
+#
+# Usage:
+# Check all .slt files under datafusion/sqllogictest/test_files:
+# ./check_slt_configs.sh
+#
+# Check a specific file (repo-relative or absolute path):
+# ./check_slt_configs.sh datafusion/sqllogictest/test_files/foo.slt
+# ./check_slt_configs.sh /absolute/path/to/foo.slt
+#
+# Exit codes:
+# 0 All checks passed
+# 1 One or more files contain configs set to true but never reset
+
+set -euo pipefail
+
+# Resolve repository root
+ROOT_DIR="$(git rev-parse --show-toplevel)"
+TARGET_DIR="${ROOT_DIR}/datafusion/sqllogictest/test_files"
+
+FAILED=0
+
+resolve_file() {
+ local input="$1"
+
+ # If absolute and exists
+ if [[ -f "$input" ]]; then
+ realpath "$input"
+ return
+ fi
+
+ # If relative to repo root
+ if [[ -f "${ROOT_DIR}/${input}" ]]; then
+ realpath "${ROOT_DIR}/${input}"
+ return
+ fi
+
+ echo ""
+}
+
+# If file argument is passed, check only that file
+if [[ $# -eq 1 ]]; then
+ RESOLVED_FILE="$(resolve_file "$1")"
+
+ if [[ -z "$RESOLVED_FILE" ]]; then
+ echo "❌ File does not exist: $1"
+ exit 1
+ fi
+
+ FILES="$RESOLVED_FILE"
+else
+ FILES=$(find "$TARGET_DIR" -type f -name "*.slt")
+fi
+
+# Iterate safely even if filenames contain spaces
+while IFS= read -r file; do
+ echo "Checking file: $file"
+
+ if [[ ! -f "$file" ]]; then
+ echo " ❌ File not found: $file"
+ FAILED=1
+ continue
+ fi
+
+ matches=$(grep -En \
+ 'set[[:space:]]+datafusion\.[a-zA-Z0-9_.]+[[:space:]]*=[[:space:]]*true' \
Review Comment:
Good catch - I missed that `SET` can appear in uppercase.
I’ll update the script to perform case-insensitive matching so both `set` and `SET` are handled correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org