diff --git a/datafusion/core/src/lib.rs b/datafusion/core/src/lib.rs index 28ea4b4490c3e..b270ab7669ff3 100644 --- a/datafusion/core/src/lib.rs +++ b/datafusion/core/src/lib.rs @@ -261,6 +261,12 @@ //! [`Expr`]s to compute the desired result by [`SqlToRel`]. This phase //! also includes name and type resolution ("binding"). //! +//! Note the parser checks *syntax* only. Semantic checks belong on the +//! [`LogicalPlan`], so they apply to plans built by the DataFrame API and other +//! frontends as well as by SQL. See the [Syntax vs Semantics] specification. +//! +//! [Syntax vs Semantics]: https://datafusion.apache.org/contributor-guide/specification/syntax-vs-semantics.html +//! //! [`Statement`]: https://docs.rs/sqlparser/latest/sqlparser/ast/enum.Statement.html //! //! ### DataFrame diff --git a/datafusion/sql/src/parser.rs b/datafusion/sql/src/parser.rs index 12c2569740c4f..8f0def1862664 100644 --- a/datafusion/sql/src/parser.rs +++ b/datafusion/sql/src/parser.rs @@ -19,6 +19,18 @@ //! //! This parser implements DataFusion specific statements such as //! `CREATE EXTERNAL TABLE` +//! +//! # Syntax vs Semantics +//! +//! Like [`sqlparser`], this parser checks *syntax* only: it answers whether the +//! text is a well formed statement, not whether that statement means anything. +//! Semantic checks (unknown columns, invalid types, contradictory options, ...) +//! belong on the [`LogicalPlan`] so that they also apply to plans built by the +//! DataFrame API, Substrait, and other frontends. See the [Syntax vs Semantics] +//! specification for where to put such checks. +//! +//! [`LogicalPlan`]: datafusion_expr::LogicalPlan +//! [Syntax vs Semantics]: https://datafusion.apache.org/contributor-guide/specification/syntax-vs-semantics.html use datafusion_common::DataFusionError; use datafusion_common::config::{ConfigNonZeroUsize, SqlParserOptions}; diff --git a/docs/source/contributor-guide/specification/index.rst b/docs/source/contributor-guide/specification/index.rst index a34f0b19e4dea..ece116aa08ab2 100644 --- a/docs/source/contributor-guide/specification/index.rst +++ b/docs/source/contributor-guide/specification/index.rst @@ -33,3 +33,4 @@ specifications: invariants output-field-name-semantic + syntax-vs-semantics diff --git a/docs/source/contributor-guide/specification/syntax-vs-semantics.md b/docs/source/contributor-guide/specification/syntax-vs-semantics.md new file mode 100644 index 0000000000000..79d73da0891a1 --- /dev/null +++ b/docs/source/contributor-guide/specification/syntax-vs-semantics.md @@ -0,0 +1,192 @@ + + +# Syntax vs Semantics + +DataFusion rejects invalid queries in several different places: the SQL parser, +the SQL planner, `LogicalPlan` constructors, the `Analyzer`, and finally during +execution. This document specifies which kind of check belongs where. + +The short version: + +- The **parser** checks **syntax** only. +- **Semantic** checks belong on the `LogicalPlan`, so they apply no matter how + the plan was built. + +## Definitions + +A **syntax** check answers "is this text a well formed statement in DataFusion's +SQL dialect?". For example, `SELECT FROM t` is not a statement, and +`CREATE EXTERNAL TABLE t STORED AS CSV` is missing its required `LOCATION` +clause. Answering such a question requires nothing but the text itself. + +A **semantic** check answers "does this well formed statement describe something +DataFusion can compute?". For example: + +- `SELECT nonexistent FROM t` (no such column) +- `SELECT * FROM t WHERE 1` (predicate is not boolean) +- `SELECT * FROM t WHERE sum(a) > 0` (aggregate in `WHERE`) +- `CREATE FUNCTION f(DOUBLE, DOUBLE) RETURNS DOUBLE RETURN $1 + $3` (no third + argument to reference) + +Answering these questions requires more than the text: the catalog, schemas, +types, and the surrounding plan. + +## The parser checks syntax only + +DataFusion parses SQL with [sqlparser-rs]; [`DFParser`] extends it with +DataFusion specific statements such as `CREATE EXTERNAL TABLE` and `COPY`. +sqlparser-rs is explicitly a syntax-only parser, as described in its +[Syntax vs Semantics] section: + +> This crate provides only a syntax parser, and tries to avoid applying any SQL +> semantics, and accepts queries that specific databases would reject, even when +> using that Database's specific `Dialect`. For example, +> `CREATE TABLE(x int, x int)` is accepted by this crate, even though most SQL +> engines will reject this statement due to the repeated column name `x`. + +`DFParser` follows the same rule: parsing produces an AST, and says nothing +about whether the statement is meaningful. Semantic checks do not belong in the +parser because: + +1. The parser has no access to what it would need to decide: the catalog, + schemas, registered functions, or session configuration. +2. The AST is also used for purposes other than planning, such as round + tripping SQL text, and rejecting statements there makes those uses + impossible. +3. A check in the parser applies only to SQL text that DataFusion parsed + itself, and is skipped by every other way of building a plan (see below). + +## Semantic checks belong on the `LogicalPlan` + +SQL is only one of the ways a `LogicalPlan` is created. Plans also come from: + +- the [DataFrame API] and [`LogicalPlanBuilder`] +- [Substrait] and the protobuf serialization in `datafusion-proto` +- other SQL dialects and query languages built on DataFusion, which may use + their own parser and planner + +A check in [`SqlToRel`] only covers the first of these. If the rule is a +property of the plan rather than of SQL text, enforce it where every frontend +must pass: the constructor of the `LogicalPlan` node (or of the structure it +carries), in the `datafusion-expr` crate. That is what `Filter::try_new`, +`Aggregate::try_new`, `Union::try_new`, and friends are for. A new statement +type should get a `try_new` that validates its own rules rather than relying on +the SQL planner to do so. + +Placing the check there also means the rule is enforced once, is tested once, +and cannot be bypassed by a rewrite rule or an extension that rebuilds the node. + +| Question the check answers | Where it belongs | +| ------------------------------------------------------------------- | ---------------------------------------------------------- | +| Is the text a well formed statement? | `DFParser` / sqlparser-rs (`datafusion/sql/src/parser.rs`) | +| Can this SQL construct be represented as a plan at all? | `SqlToRel` (`datafusion/sql/src/`) | +| Does this plan node make sense (types, arity, references, options)? | `try_new` on the node in `datafusion/expr/` | +| Does the rule need the whole plan, or types after coercion? | An `AnalyzerRule`, or a [plan invariant](invariants.md) | +| Can it only be known from the data itself? | Execution (for example, an overflowing cast) | + +### What stays in the SQL planner + +[`SqlToRel`] converts the AST into a `LogicalPlan`, including name and type +resolution ("binding"). Two kinds of errors legitimately originate there: + +- **Unsupported SQL**: a clause DataFusion has no plan representation for, such + as `INSERT ... RETURNING`. There is no plan node to validate, so the SQL + planner is the only place the error can be raised. +- **SQL specific context**: spans and [`Diagnostic`]s that point at the offending + text. Note this is context added to an error, not the check itself. + +### Checks that need the whole plan + +Some rules cannot be decided when a single node is built, for example because +they depend on types after coercion, or on the relationship between a subquery +and its outer plan. Those belong in an `AnalyzerRule` (such as type coercion) or +in the logical plan invariants, described in [Invariants](invariants.md) and +implemented in `datafusion/expr/src/logical_plan/invariants.rs`. + +## Testing semantic checks + +Prefer an end to end [sqllogictest] (`.slt`) case for the user visible error, +rather than only a unit test of the function that happens to implement the check +today. End to end tests keep passing when a check moves to a different layer, +which is exactly the refactoring this document encourages. When a rule applies +to plans built directly, add a test that builds the plan with the DataFrame API +or `LogicalPlanBuilder` as well. + +## Examples + +### Checks in the right place + +- **Non boolean filter predicate** and **window functions in a filter + predicate**: both are enforced in `Filter::try_new` + (`datafusion/expr/src/logical_plan/plan.rs`), so `WHERE` in SQL and + `DataFrame::filter` report the same error at the same point: + + ```text + // SQL: SELECT * FROM t WHERE 1 + Error during planning: Cannot create filter with non-boolean predicate 'Int64(1)' returning Int64 + + // DataFrame: df.filter(lit(1)) + Error during planning: Cannot create filter with non-boolean predicate 'Int32(1)' returning Int32 + ``` + +- **Nested aggregate or window calls**: `check_aggregate_and_window_nesting` + (`datafusion/expr/src/utils.rs`), called from `Aggregate::try_new` and + `Window::try_new`. +- **Mismatched `UNION` inputs**: `Union::try_new`. +- **Type coercion**: the `TypeCoercion` `AnalyzerRule`, which runs on every plan + regardless of how it was built. +- **Invalid correlated subqueries**: `assert_valid_semantic_plan` + (`datafusion/expr/src/logical_plan/invariants.rs`). + +### Checks in the wrong place + +These are existing violations, kept here as illustrations. They are not +precedents for new code, and PRs moving them are welcome. + +- **Aggregates in `WHERE`** are rejected in the SQL planner + (`datafusion/sql/src/select.rs`), unlike window functions in `WHERE`, which + are rejected in `Filter::try_new`. The consequence is visible from the + DataFrame API, which builds the invalid plan and only fails later, during + physical planning, with a much less helpful message: + + ```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 + ``` + +- **`CREATE EXTERNAL TABLE` clause combinations**, such as + `'IF NOT EXISTS' cannot coexist with 'REPLACE'` and + `Constraints on Partition Columns are not supported`, are rejected by + `DFParser` (`datafusion/sql/src/parser.rs`) although both statements parse. +- **Column references in a `DEFAULT` expression** are rejected in `SqlToRel` + (`datafusion/sql/src/planner.rs`) rather than when the DDL node is built. + +[sqlparser-rs]: https://github.com/apache/datafusion-sqlparser-rs +[syntax vs semantics]: https://github.com/apache/datafusion-sqlparser-rs#syntax-vs-semantics +[`dfparser`]: https://docs.rs/datafusion/latest/datafusion/sql/parser/struct.DFParser.html +[`sqltorel`]: https://docs.rs/datafusion/latest/datafusion/sql/planner/struct.SqlToRel.html +[`logicalplanbuilder`]: https://docs.rs/datafusion/latest/datafusion/logical_expr/struct.LogicalPlanBuilder.html +[`diagnostic`]: https://docs.rs/datafusion/latest/datafusion/common/struct.Diagnostic.html +[dataframe api]: ../../user-guide/dataframe.md +[substrait]: https://docs.rs/datafusion-substrait/latest/datafusion_substrait/ +[sqllogictest]: ../testing.md#sqllogictests-tests diff --git a/docs/source/library-user-guide/extending-sql.md b/docs/source/library-user-guide/extending-sql.md index 120bc8fab21a5..3eb2112d05fa7 100644 --- a/docs/source/library-user-guide/extending-sql.md +++ b/docs/source/library-user-guide/extending-sql.md @@ -56,6 +56,12 @@ The extension planners intercept specific parts of the SQL AST during the `SqlToRel` phase and allow you to customize how they are converted to DataFusion's logical plan. +The `Parser` stage checks syntax only, and validation that depends on schemas, +types, or the catalog belongs on the `LogicalPlan` rather than in the parser or +the SQL planner. See [Syntax vs Semantics] for where to put such checks. + +[syntax vs semantics]: ../contributor-guide/specification/syntax-vs-semantics.md + ## Extension Points DataFusion provides three planner traits for extending SQL: