kosiew commented on code in PR #24493:
URL: https://github.com/apache/datafusion/pull/24493#discussion_r3852666423
##########
xtask/src/ci_steps.rs:
##########
@@ -353,6 +353,7 @@ impl StepContext {
"--",
"--substrait-round-trip",
"limit.slt",
+ "configmatrix_show_smoke.slt",
Review Comment:
I think the regression coverage is still missing here.
`configmatrix_show_smoke.slt` is passed as a CI filter, but there is no tracked
file with that name under `datafusion/sqllogictest/test_files`.
Because unmatched file filters are silently ignored, this step still only
runs `limit.slt`, so no matrix directive reaches the Substrait round-trip path.
That means the earlier request for a regression test that proves every matrix
combination is executed is not actually covered yet.
Could we either add the missing smoke `.slt`, ideally with alternatives
where skipping either combination would produce an observable failure, or point
this filter at an existing matrix-bearing test file?
##########
datafusion/sqllogictest/bin/sqllogictests.rs:
##########
@@ -922,6 +1033,14 @@ struct Options {
)]
timing_summary: bool,
+ #[clap(
+ long,
+ env = "SLT_TIMING_DEBUG_SLOW_FILES",
+ default_value_t = false,
Review Comment:
This looks like a compatibility regression in the environment variable
handling. The previous `SLT_TIMING_DEBUG_SLOW_FILES` parser accepted `1`,
`true`, `yes`, and `on` case-insensitively, while other values were treated as
disabled. With Clap's bool parser, only `true` and `false` are accepted, and a
previously valid value such as `1` now aborts the entire test binary with a
usage error.
I verified this with `SLT_TIMING_DEBUG_SLOW_FILES=1 cargo test -p
datafusion-sqllogictest --test sqllogictests --features substrait -- --list
--format terse`, which exits 2 with `invalid value '1'`.
Could we preserve the previous parsing semantics, for example with a custom
Clap value parser, or keep the existing environment-only parser and expose the
CLI flag separately?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
