Adds basic scalar and row-value subquery coercion #1258
Conversation
Conformance comparison report
Number passing in both: 5372 Number failing in both: 446 Number passing in Base (262074f) but now fail: 0 Number failing in Base (262074f) but now pass: 0 |
| --#[subquery-02] | ||
| SELECT t.*, s.* | ||
| FROM T AS t | ||
| JOIN (SELECT * FROM S) AS s | ||
| ON t.x = s.a; |
| --#[subquery-00] | ||
| SELECT x | ||
| FROM T | ||
| WHERE x IN (SELECT a FROM S); | ||
|
|
||
| --#[subquery-01] | ||
| SELECT x | ||
| FROM T | ||
| WHERE x > (SELECT MAX(a) FROM S); |
There was a problem hiding this comment.
While this PR hasn't added full support for ROW coercion yet, would you also want to include tests where the subquery has more than 1 column?
There was a problem hiding this comment.
Yes, working on that now. Had some transpiler cases for this which are failing.
| private fun visitExprCoerce(node: Expr, ctx: Env, coercion: Rex.Op.Subquery.Coercion = Rex.Op.Subquery.Coercion.SCALAR): Rex { | ||
| val rex = super.visitExpr(node, ctx) | ||
| return when (rex.op is Rex.Op.Select) { | ||
| true -> rex(StaticType.ANY, rexOpSubquery(rex.op as Rex.Op.Select, coercion)) | ||
| else -> rex | ||
| } | ||
| } |
There was a problem hiding this comment.
1:
Is there a reason why we don't make the default visitExpr have this logic? And whenever we encounter the limited cases (FROM, JOIN, etc), we invoke a function called visitExprNoCoercion.
2:
If there is a reason, do we need to pass coercion here?
An SELECT SFW subquery coerces into an array when it is the rhs (respectively, lhs) of a comparison operator whose other argument is an array.
The spec doesn't say array literal. Which means, we don't know whether to coerce the RHS into an array until we have typed.
3:
That being said, I'm not sure I'm a fan of the:
... operator whose other argument is an array.
To be SQL-compatible, I feel like we should syntactically support coercion to an array where the other argument is:
- an array constructor:
ARRAY [ ... ] - a row constructor:
ROW ( ... ),( ..., ... ) - a subquery:
(SELECT ...)
Seems a bit weird to allow:
SELECT a
FROM t
LET [ t.a, t.b ] AS c
WHERE c < (SELECT d, e FROM g)
| // val n = coercion.columns.size | ||
| // val m = cons.fields.size | ||
| // if (n != m) { | ||
| // return rexErr("Cannot coercion subquery with $m attributes to a row-value-expression with $n attributes") |
There was a problem hiding this comment.
Depending on how we interpret the spec and whether coercion only pertains to array literals (it currently says arrays -- not literals), then we can potentially perform the cardinality check on the AST instead, which would be simpler.
There was a problem hiding this comment.
I agree; I actually had this check in the RexConverter rather than the typer because I had access to the AST. I moved the to PlanTyper to perform type assertions on the constructor, but I'm beginning to think we may be limited to array expressions (not array literals).
There was a problem hiding this comment.
The cardinality check is just as easy for array expressions and literals, and we need the typing of the subquery for cases like
( 0, 0 ) = (SELECT * FROM points LIMIT 1); -- check if the first point is the origin
-- points : bag<<struct::closed{ x: int32, y: int32 }>>
RCHowell
force-pushed
the
planner-subquery
branch
from
52dab2a
to
786d861
Compare
Relevant Issues
Description
Adds basic scalar and row-value scalar subquery coercion. Note that this currently does not consider array expressions for row-value. A spot has been carved out, but is not required at the moment.
Other Information
Updated Unreleased Section in CHANGELOG: [YES/NO]
No
Any backward-incompatible changes? [YES/NO]
No
Any new external dependencies? [YES/NO]
No
Do your changes comply with the Contributing Guidelines
and Code Style Guidelines? [YES/NO]
Yes
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.