-
Notifications
You must be signed in to change notification settings - Fork 71
add table references and "this" to SQL expressions #6292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
compiler/semantic/expr.go
Outdated
| Where: t.semExprNullable(call.Where), | ||
| Node: n, | ||
| Name: name, | ||
| Expr: e, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
| Expr: e, | |
| Expr: t.semExprNullable(arg), |
|
Here's a failure that happens on the branch that runs ok on tip of |
|
Here's one that runs on the branch but produces only error outputs. It uses the same test data as in #6292 (comment). Whereas on tip of |
This commit adds support for referring to tables in a SQL expression, resulting in a record representing the table row (as in duckdb and somewhat similarly in postgres). We also added support for referencing "this" in a SQL expression, which refers to the input relation in SELECT and WHERE expressions, the output relation in HAVING clauses, and the input relation for arguments (and where clauses) of agg functions in HAVING clauses. This new logic causes an error for table references of dynamic schemas. This is to avoid a situation where "select T from T" refers to the table in a dynamic schema, then when a schema shows up for that same data, the query compiles differently to a field reference of T inside T (following postgres and duckdb scope precedence). When it is desied to refer to the table of a dynamic source, the special value "this" can be used instead. In general, query semantics should be identical when types/schemas are known and unknown; if this isn't the case anywhere here, then it's a design bug. We also added an escape valve for referring to a SQL column named "this", which is simply denoted with double quotes. Finally, these changes exposed a problem in the as-name inference algorithm, where the internal DAG paths would show up, so we updated the inference code to strictly use the AST instead of a mix of the AST and sem tree.
ba3a6c0 to
7428ee7
Compare
|
@mccanne and @nwt spotted that the failures in my two comments above could both be explained by the absence of the |
|
Following up on the prior comment, after adding the additional |
This commit adds support for referring to tables in a SQL expression, resulting in a record representing the table row (as in duckdb and somewhat similarly in postgres). We also added support for referencing "this" in a SQL expression, which refers to the input relation in SELECT and WHERE expressions, the output relation in HAVING clauses, and the input relation for arguments (and where clauses) of agg functions in HAVING clauses.
This new logic causes an error for table references of dynamic schemas. This is to avoid a situation where "select T from T" refers to the table in a dynamic schema, then when a schema shows up for that same data, the query compiles differently to a field reference of T inside T (following postgres and duckdb scope precedence). When it is desied to refer to the table of a dynamic source, the special value "this" can be used instead. In general, query semantics should be identical when types/schemas are known and unknown; if this isn't the case anywhere here, then it's a design bug.
We also added an escape valve for referring to a SQL column named "this", which is simply denoted with double quotes.
Finally, these changes exposed a problem in the as-name inference algorithm, where the internal DAG paths would show up, so we updated the inference code to strictly use the AST instead of a mix of the AST and sem tree.
Fixes #6241