Skip to content
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

Adds support for ROW and SCALAR subquery coercion #1356

Closed
wants to merge 1 commit into from

Conversation

johnedquinn
Copy link
Member

@johnedquinn johnedquinn commented Jan 29, 2024

Relevant Issues

  • None

Description

  • Adds support for ROW and SCALAR subquery coercion
  • NOTE: There is a bug with correlated subqueries (not subquery coercion which is this PR) that doesn't let us handle references to variables in the outer scope. See Adds support for PERMISSIVE vs STRICT #1353 (comment). I've looked further into it, and it is likely going to require an overhaul of planning/evaluation. I've decided to exclude this from this PR, as this is specifically for coercion.

How PR accomplishes this

  • According to the PartiQL Specification, when SQL's SELECT is seen in a query (not on the RHS of an IN, not on the RHS of a FROM, and not as a top-level expression), it is coerced to a scalar. It does this by grabbing the first attribute of the first value (a tuple) of the SQL SELECT. However, if it is on the LHS or RHS of a literal array, then it gets coerced into a LIST. It does this by grabbing all attribute of the first value (a tuple) of the SQL SELECT.
  • In order to accomplish this, we need to maintain the original query's semantics upon typing the plan. That being said, we cannot preemptively normalize a SQL SELECT to a PARTIQL SELECT VALUE. We need to first gather sufficient information to determine whether a SELECT should be coerced at all (and if so, whether it should be a SCALAR or ROW coercion). This PR moves the NormalizeSelect to occur during the RelConverter's transformation.
  • With this change, at planning time, we have a clear understanding of the user's intentions while coercing subqueries.

Other Information

  • Updated Unreleased Section in CHANGELOG: NO
  • Any backward-incompatible changes? Potentially
    • Potentially yes for behavioral changes. NormalizeSelect no longer occurs in the org.partiql.ast.normalize API.
  • Any new external dependencies? NO
  • Do your changes comply with the Contributing Guidelines
    and Code Style Guidelines? YES

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (partiql-plugin-impl@101c19b). Click here to learn what that means.

Additional details and impacted files
@@                  Coverage Diff                   @@
##             partiql-plugin-impl    #1356   +/-   ##
======================================================
  Coverage                       ?   49.27%           
  Complexity                     ?     1046           
======================================================
  Files                          ?      166           
  Lines                          ?    13395           
  Branches                       ?     2504           
======================================================
  Hits                           ?     6600           
  Misses                         ?     6138           
  Partials                       ?      657           
Flag Coverage Δ
CLI 11.86% <0.00%> (?)
EXAMPLES 80.28% <0.00%> (?)
LANG 54.71% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Jan 29, 2024

Conformance comparison report-Cross Engine

Base (legacy) eval +/-
% Passing 92.54% 26.54% -66.00%
✅ Passing 5384 1544 -3840
❌ Failing 434 4274 3840
🔶 Ignored 0 0 0
Total Tests 5818 5818 0
Number passing in both: 1541

Number failing in both: 431

Number passing in legacy engine but fail in eval engine: 3843

Number failing in legacy engine but pass in eval engine: 3
⁉️ CONFORMANCE REPORT REGRESSION DETECTED ⁉️
The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.
The following test(s) are failing in legacy but pass in eval. Before merging, confirm they are intended to pass:

Click here to see
  • nullif valid cases{first:"missing",second:"missing",result:missing}, compileOption: PERMISSIVE

  • nullif valid cases{first:"missing",second:"missing",result:missing}, compileOption: LEGACY

  • missing and true, compileOption: PERMISSIVE

Conformance comparison report-Cross Commit-LEGACY

Base (c16584f) c340fdb +/-
% Passing 92.54% 92.54% 0.00%
✅ Passing 5384 5384 0
❌ Failing 434 434 0
🔶 Ignored 0 0 0
Total Tests 5818 5818 0
Number passing in both: 5384

Number failing in both: 434

Number passing in Base (c16584f) but now fail: 0

Number failing in Base (c16584f) but now pass: 0

Conformance comparison report-Cross Commit-EVAL

Base (c16584f) c340fdb +/-
% Passing 26.54% 26.54% 0.00%
✅ Passing 1544 1544 0
❌ Failing 4274 4274 0
🔶 Ignored 0 0 0
Total Tests 5818 5818 0
Number passing in both: 1544

Number failing in both: 4274

Number passing in Base (c16584f) but now fail: 0

Number failing in Base (c16584f) but now pass: 0

@johnedquinn johnedquinn force-pushed the partiql-eval-coercion branch from cda3869 to 24ffa4d Compare January 30, 2024 22:51
Comment on lines +17 to +42
internal class Row(
override val subquery: Operator.Expr
) : ExprSubquery() {
@PartiQLValueExperimental
override fun eval(record: Record): PartiQLValue {
val values = getFirstAndOnlyTupleValues(record)
return listValue(values.asSequence().toList())
}
}

internal class Scalar(
override val subquery: Operator.Expr
) : ExprSubquery() {
@PartiQLValueExperimental
override fun eval(record: Record): PartiQLValue {
val values = getFirstAndOnlyTupleValues(record)
if (values.hasNext().not()) {
throw TypeCheckException()
}
val singleValue = values.next()
if (values.hasNext()) {
throw TypeCheckException()
}
return singleValue
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the subquery an Expression and not a Relation? No need to evaluate the whole expression

@PartiQLValueExperimental
override fun eval(record: Record): PartiQLValue {
val values = getFirstAndOnlyTupleValues(record)
return listValue(values.asSequence().toList())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why list and not struct? PartiQL's "row" is the binding-tuple —> struct in the value domain

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, the specification is inconsistent on this

First it says tuple,

Furthermore, when SELECT is used as a subquery it is coerced into a scalar or a tuple,
in the ways that SQL coerces the results of subqueries. p.24

Then it says array,

SFW subquery starting with a SELECT clause (as opposed to a subquery starting with SELECT VALUE or PIVOT) coerces into a scalar or into an array, depending on the context p33

Then it says tuple again on page 33

An PartiQL extension with respect to SQL is that, in the permissive mode, subqueries that fail to coerce to the
required type (scalar or tuple) still run, as opposed to failing

It makes sense to coerce to array (list) for predicates with array literals, but perhaps a more important question is what is PartiQL's equivalent to the SQL ROW type? Or was the term "tuple" used to mean an ordered collection and not the overloaded "tuple" value.

The specification explicitly calls struct values tuple values (and the other complex values collection ie bag, array). I'm curious now which type in the data model is equivalent to the SQL ROW value — list or struct

@johnedquinn
Copy link
Member Author

Closing in favor of #1365

@johnedquinn johnedquinn closed this Feb 9, 2024
@johnedquinn johnedquinn deleted the partiql-eval-coercion branch February 9, 2024 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants