alamb opened a new pull request, #25540: URL: https://github.com/apache/datafusion/pull/25540
## Which issue does this PR close? - No issue; this follows from a PR review discussion: https://github.com/apache/datafusion/pull/25039#discussion_r4056745517 ("it isn't explicitly stated anywhere in the DataFusion docs. I will draft a PR to do so") ## Rationale for this change When reviewing PRs we regularly ask contributors to move a check out of the SQL parser or the SQL planner, because: 1. The parser checks syntax, not meaning (this is [stated explicitly by sqlparser](https://github.com/apache/datafusion-sqlparser-rs#syntax-vs-semantics), but nowhere in DataFusion's own docs) 2. A semantic check made in `SqlToRel` does not apply to plans built by the DataFrame API, Substrait, `datafusion-proto`, or other query languages built on DataFusion Today the only statement of this in DataFusion is implicit, in the [planning overview](https://docs.rs/datafusion/latest/datafusion/index.html#query-planning-and-execution-overview), which describes where each phase happens but never says which kind of check belongs where. That makes the review feedback look like a matter of taste, and new checks keep landing in the wrong layer. ## What changes are included in this PR? A new specification page, `Syntax vs Semantics` (`docs/source/contributor-guide/specification/syntax-vs-semantics.md`), which: - Defines syntax vs semantic checks, with examples of each - States that `DFParser` checks syntax only, and why (no catalog/schema access, the AST is also used for round tripping, and parser checks are skipped by every non-SQL frontend), quoting sqlparser's `Syntax vs Semantics` section - States that semantic checks belong on the `LogicalPlan`, in a node's `try_new` in `datafusion-expr`, so that every frontend passes through them - Includes a table mapping each kind of check to its layer (parser, `SqlToRel`, `try_new`, `AnalyzerRule`/invariants, execution) - Describes what legitimately stays in `SqlToRel` (unsupported SQL constructs that have no plan representation, and `Diagnostic`/span context on errors) - Recommends `.slt` tests for the user visible error, so that tests survive a check moving between layers - Lists real examples from the codebase, both checks in the right place and checks in the wrong place It also cross links the new page from the three places a contributor is likely to be when they write such a check: the `DFParser` docs, the crate level architecture docs in `datafusion/core/src/lib.rs`, and the "Extending SQL" library user guide. ### Examples in the doc In the right place (single check, all frontends): - `Filter::try_new`: non boolean predicates and window functions in a predicate - `check_aggregate_and_window_nesting`, from `Aggregate::try_new` / `Window::try_new` - `Union::try_new`, the `TypeCoercion` analyzer rule, `assert_valid_semantic_plan` In the wrong place (SQL only), listed as existing violations rather than precedents: - Aggregates in `WHERE` (`datafusion/sql/src/select.rs`). The same query via the DataFrame API builds the plan and fails much later in physical planning: ```text // SQL: SELECT * FROM t WHERE sum(a) > 0 Error during planning: Aggregate functions are not allowed in the WHERE clause. Consider using HAVING instead // DataFrame: df.filter(sum(col("a")).gt(lit(0))) Error during planning: Aggregate function 'sum(CAST(t.a AS Int64))' is not supported in this position. Aggregate functions are supported in the SELECT list, HAVING and ORDER BY of a query with GROUP BY ``` Contrast with window functions in `WHERE`, checked in `Filter::try_new`, where SQL and the DataFrame API give the same error. - `'IF NOT EXISTS' cannot coexist with 'REPLACE'` and `Constraints on Partition Columns are not supported`, rejected in `DFParser` although both statements parse - `Column reference is not allowed in the DEFAULT expression`, rejected in `SqlToRel` ## What is the testing strategy for this PR? Docs only, so there are no new tests. `./ci/scripts/doc_prettier_check.sh`, `cargo fmt --all -- --check`, and `RUSTDOCFLAGS="-D warnings" cargo doc -p datafusion-sql --no-deps` (for the new intra doc link) all pass. The error messages quoted in the new page were produced by running the queries against this branch rather than written from memory. ## Are there any user-facing changes? No code changes. New contributor documentation, plus rustdoc pointers to 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: [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]
