kosiew commented on code in PR #24493:
URL: https://github.com/apache/datafusion/pull/24493#discussion_r3861728492
##########
datafusion/sqllogictest/bin/sqllogictests.rs:
##########
@@ -443,33 +443,48 @@ async fn run_test_file_substrait_round_trip(
path,
relative_path,
} = test_file;
- let Some(test_ctx) =
TestContext::try_new_for_test_file(&relative_path).await else {
- info!("Skipping: {}", path.display());
- return Ok(());
- };
- setup_scratch_dir(&relative_path)?;
- let count: u64 = get_record_count(&path,
"DatafusionSubstraitRoundTrip".to_string());
- let pb = mp.add(ProgressBar::new(count));
+ // Parsed once and replayed for every configuration.
+ let records = parse_records(&path)?;
+ let count = count_records(&records, "DatafusionSubstraitRoundTrip");
- pb.set_style(mp_style);
- pb.set_message(relative_path.display().to_string());
+ for test_configuration in test_configurations(&path)? {
+ let Some(test_ctx) =
TestContext::try_new_for_test_file(&relative_path).await
+ else {
+ info!("Skipping: {}", path.display());
+ return Ok(());
+ };
+ setup_scratch_dir(&relative_path)?;
+ // Before the engine is built: it snapshots config to detect drift.
+ test_ctx.apply_config_overrides(test_configuration.settings(),
&relative_path)?;
+
+ let pb = mp.add(ProgressBar::new(count));
+ pb.set_style(mp_style.clone());
+ pb.set_message(relative_path.display().to_string());
+
+ let mut runner = sqllogictest::Runner::new(|| async {
+ Ok(DataFusionSubstraitRoundTrip::new(
+ test_ctx.session_ctx().clone(),
+ relative_path.clone(),
+ pb.clone(),
+ )
+ .with_currently_executing_sql_tracker(
+ currently_executing_sql_tracker.clone(),
+ ))
+ });
+ runner.add_label("DatafusionSubstraitRoundTrip");
+ runner.with_column_validator(strict_column_validator);
+ runner.with_normalizer(value_normalizer);
+ runner.with_validator(validator);
+ let result =
+ run_file_in_runner(&path, &records, &mut runner, filters,
colored_output)
+ .await;
+ pb.finish_and_clear();
+
+ test_configuration.attribute_failure(result)?;
Review Comment:
It looks like a failure in the first Substrait matrix configuration is still
propagated with `?`, so the remaining matrix combinations never run. The
default runner appears to have the same behavior around line 562.
That means a file does not actually run once for every combination when an
earlier combination fails, and it also makes it hard to see all
configuration-specific failures in one run.
Could we keep executing the remaining configurations and aggregate the
failures with enough context to identify the configuration that produced each
one? I would also like to see a tracked matrix SLT or integration test where
skipping a later combination is observable, so this behavior is covered for the
Substrait path as well.
##########
datafusion/sqllogictest/src/test_context.rs:
##########
@@ -232,6 +232,34 @@ impl TestContext {
pub fn session_ctx(&self) -> &SessionContext {
&self.ctx
}
+
+ /// Apply a set of `key = value` config overrides to the underlying
+ /// `SessionContext` in-place. Used by the sqllogictest runner to sweep
+ /// `# configMatrix:` directives; keeps the session-state internals
+ /// hidden from callers.
+ ///
+ /// `origin` is a display label for error messages (typically the test
+ /// file path).
+ ///
+ /// Note this sets config options directly rather than going through
Review Comment:
I think this still leaves an important gap in what `configMatrix` can
configure. The comment notes that these overrides are applied directly to
`ConfigOptions` instead of going through the `SessionContext` `SET` path, but
the README presents `configMatrix` more generally as a way to sweep
configuration values.
For example, a matrix containing `datafusion.runtime.memory_limit=100M`
currently fails with `Config value "runtime" not found on ConfigOptions`, even
though `SET datafusion.runtime.memory_limit = '100M'` is supported.
Could we route matrix overrides through the same runtime-config and
config-dependent UDF refresh path used by `SessionContext::set_variable`, or
extract a shared helper for both paths? It would also be good to add coverage
for a runtime setting and any config-dependent UDF behavior. If matrices are
intentionally limited to `ConfigOptions`, then I think that narrower scope
should be explicitly enforced and documented.
--
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]
