kosiew opened a new issue, #24719: URL: https://github.com/apache/datafusion/issues/24719
## Related PR #23975 ## Problem The statistics runner snapshots `sql_parser` options once at startup and uses that snapshot to parse every SQL file. `SET` statements executed from an earlier file mutate the `SessionContext` configuration, but later files are still parsed with the stale startup options. [Current behavior](https://github.com/apache/datafusion/tree/a38bb10c33cff4e99a2707059df99f6971c96f33) in benchmarks/src/statistics.rs: - RunOpt::run clones parser options once from ctx.state().config_options().sql_parser. - Each file is then parsed by sql_statements(sql, &sql_parser_options). - Session statements that mutate parser configuration (for example SQL dialect or parser recursion limit) do not affect parsing of subsequent files. This can fail valid suites where parsing requirements intentionally change over time, such as setting the MySQL dialect before parsing MySQL-specific SQL in a later file. Query files are processed deterministically (numeric filename component, then path), so cross-file configuration changes have a defined order. ## Why it matters - Correctness: suite behavior diverges from session semantics. - Predictability: statement execution and pre-parsing use different configuration timelines. - Maintenance: users get parse failures that are hard to explain because runtime session state appears correctly updated. ## Invariant / desired behavior Each query file must be parsed with parser settings read from the current `SessionContext` state immediately before that file is parsed. A parser-setting change successfully executed in an earlier query file must affect parsing of every later query file. ## Proposed direction Move the parser-option lookup to the file boundary: - Remove the one-time options snapshot in `RunOpt::run`. - Immediately before parsing each file, clone `ctx.state().config_options().sql_parser`. - Parse that file with the refreshed options. Keep scope intentionally narrow: - Preserve current flow of parsing a file into statements before executing statements from that same file. - Do not introduce full statement-by-statement re-parsing within a file unless a concrete use case requires it. Also add explicit docs/comments for the still-supported limitation: - Intra-file parser-setting changes do not retroactively reparse later statements already parsed from the same file. ## Scope ### In - File-level parser option refresh for statistics runner. - Tests proving cross-file parser updates are honored. - Brief documentation note on remaining intra-file limitation. ### Out - Global parser architecture refactor. - Incremental or streaming SQL parser for mid-file setting changes. - Changes outside statistics benchmark runner. ## Acceptance criteria - [ ] Parsing options are fetched from current SessionContext state before parsing each SQL file. - [ ] A regression test with ordered temporary files demonstrates: file A sets the MySQL dialect; file B contains MySQL-specific SQL; file B parses and executes successfully. - [ ] A regression test with ordered temporary files demonstrates: file A lowers the parser recursion limit; file B contains a query exceeding that limit; file B is reported as a parse failure. - [ ] Code or docs clearly state remaining limitation for intra-file parser-setting changes. ## Tests / verification - Add focused tests near `benchmarks/src/statistics.rs` tests using ordered temporary query files (for example, `01.sql` and `02.sql`): - Cross-file dialect propagation, including execution of the later file. - Cross-file recursion-limit propagation, asserting the later file's parse failure is reported. - Keep runner/report output and Git-branch dependencies isolated in tests; extract a focused file-processing helper if exercising `RunOpt::run` directly is impractical. - Run: - cargo test -p datafusion-benchmarks --lib statistics::tests -- --nocapture - cargo check -p datafusion-benchmarks --lib ## Notes / open questions - No current benchmark suite is known to depend on cross-file parser-setting changes; this is a correctness regression test for supported session semantics. - A Rust doc comment on `RunOpt` is sufficient for the intra-file limitation; no benchmark README change is needed unless user-facing runner behavior is documented there. -- 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]
