kosiew commented on code in PR #24878: URL: https://github.com/apache/datafusion/pull/24878#discussion_r3930901469
########## datafusion/sqllogictest/test_files/config_matrix.slt: ########## @@ -0,0 +1,63 @@ +# 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. + +# End-to-end cover for the `# configMatrix:` runner path added in #24493. +# config_matrix.rs unit-tests directive parsing and expansion, and +# apply_config_overrides is tested against a context directly. Neither reaches +# the wiring that builds a fresh context per combination and applies that +# combination's overrides before the engine snapshots config. +# +# Both knobs are swept to values the default is not, so a combination whose +# overrides never landed fails: batch_size reads 8192, and time_zone reads NULL, +# which renders the cast below with no offset at all. The expected value is +# `true` in all four combinations without any of them asserting the same thing, +# because the time zone query compares the rendered instant against the setting +# that produced it rather than against a fixed string. +# +# What this cannot see: a runner that applied one combination four times still +# passes, because each replay is independent and a file has no cross-run state. +# run_each_configuration_runs_every_combination_past_failures in config_matrix.rs +# pins that half. +# +# Matrix rules: no EXPLAIN, no in-file SET of a swept knob. + +# configMatrix: datafusion.execution.batch_size=1,2 Review Comment: 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: ```text # 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: ```text 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. -- 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]
