Apply SQL 9.3 typing rules for CASE-WHEN #1391

RCHowell · 2024-03-14T23:02:06Z

Relevant Issues

Description

This PR introduces the typing rules of a CASE-WHEN which types branches to the minimum common supertype. In the case of types from different sub-type families (STF) then we go up to the ANY type (known now as dynamic).

Other Information

Updated Unreleased Section in CHANGELOG: [YES/NO]
No
Any backward-incompatible changes? [YES/NO]
This is a behavioral change due to a bug.
Any new external dependencies? [YES/NO]
No
Do your changes comply with the Contributing Guidelines
and Code Style Guidelines? [YES/NO]

License Information

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

github-actions · 2024-03-14T23:07:23Z

Conformance comparison report

	Base (`6634bc0`)	`f57111f`	+/-
% 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 (6634bc0) but now fail: 0

Number failing in Base (6634bc0) but now pass: 0

codecov-commenter · 2024-03-15T18:24:02Z

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 73.14%. Comparing base (6634bc0) to head (6bb0cb6).
Report is 1 commits behind head on main.

Files	Patch %	Lines
...artiql-cli/src/main/kotlin/org/partiql/cli/Main.kt	0.00%	7 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1391      +/-   ##
============================================
- Coverage     73.17%   73.14%   -0.03%     
  Complexity     2393     2393              
============================================
  Files           247      247              
  Lines         17616    17623       +7     
  Branches       3175     3176       +1     
============================================
  Hits          12890    12890              
- Misses         3849     3856       +7     
  Partials        877      877

Flag	Coverage Δ
CLI	`11.82% <0.00%> (-0.05%)`	⬇️
EXAMPLES	`80.07% <ø> (ø)`
LANG	`81.04% <ø> (ø)`

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.

johnedquinn · 2024-03-18T17:58:44Z

partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/PlanTyper.kt

+                when (boolOrNull(condition.op)) {
+                    true -> return branch.rex // fold


I think there's a bug here. Let's say a CASE has three branches (the first two being booleans (but not constants) and the third being a boolean of literal true). With this logic, it seems like we'd accumulate the first two visited branches, but for the third, we'd immediately return it's result.

Seems like we can only constant fold and return early if it's the first boolean and it's the boolean literal true.

Also wondering if we should still check the other branches to report any incompatible types (i.e. non-boolean WHEN branches).

I'm going to leave constant folding true out of this considering the urgency. The task of constant folding (which would include branches) is much larger than this PR.

yliuuuu

Had a high level question:

Consider: a: unionOf(int2, int4, null).

Given a query:

Case when a IS NULL then a
ELSE a END;

Should the common super type be
unionOf(int2, int4, null) // a dynamic with constraint being int2, int4 and null
or
unionOf(int4, null). // a dynamic with constraint being int4, and null

If the later, should the query a also has the type unionOf(int4, null)?

I understand that the common super type rules applied for an aggregation over values of compatible data types.

We might need to clarify what is an aggregation in PartiQL. That is: one can argue that a simple project is an aggregation. In SQL that is an aggregation over values of same data type, in PartiQL it might be an aggregation over any possible data type.

yliuuuu · 2024-03-18T17:37:13Z

partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/DynamicTyper.kt

+        val type = supertype.toNonNullStaticType()
+        val mapping = args.map { it to supertype }
+        return if (modifiers.isEmpty()) {
+            type to mapping
+        } else {
+            StaticType.unionOf(setOf(type) + modifiers).flatten() to mapping
+        }


If one would keep accumulating MISSING, does not seem like the NULL type will be dropped.

Could you please elaborate more?

val typer = DynamicTyper() // at this stage, super Type is NULL typer.accumulate(MISSING) // no touch on super type, missable is true // modifier contains missing, // type is supertype.toNonNullStaticType() -> NULL // return is StaticType.unionOf(setOf(type) + modifiers).flatten() to mapping typer.mapping() // UnionOf( NULL, MISSING)

partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/DynamicTyper.kt

partiql-cli/src/main/kotlin/org/partiql/cli/Main.kt

partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/PlanTyper.kt

alancai98 · 2024-03-18T18:31:40Z

partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/PlanTyper.kt

+                when (boolOrNull(condition.op)) {
+                    true -> return branch.rex // fold


Seems like we can only constant fold and return early if it's the first boolean and it's the boolean literal true.

Also wondering if we should still check the other branches to report any incompatible types (i.e. non-boolean WHEN branches).

partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/DynamicTyper.kt

partiql-planner/src/test/kotlin/org/partiql/planner/internal/typer/functions/NullIfTest.kt

partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/DynamicTyper.kt

partiql-planner/src/testFixtures/resources/catalogs/default/pql/t_item.ion

partiql-planner/src/testFixtures/resources/inputs/basics/case.sql

alancai98 · 2024-03-18T23:14:26Z

partiql-planner/src/testFixtures/resources/inputs/basics/case.sql

+-- type: (int32|int64|string)
+CASE t_item.t_string
+    WHEN 'a' THEN t_item.t_int32
+    WHEN 'b' THEN t_item.t_int64
+    ELSE 'default'
+END;


So for the heterogeneous cases, we're opting to keep all of the return types in the minimal common subtype and not doing any sort of "flattening" (for this case, returning (int64, string))?

Exactly. Once the minimal common supertype hits the top ie dynamic, we don't coerce/cast and branches. We carry all types to the result.

Co-authored-by: Alan Cai <[email protected]>

yliuuuu

LGTM. Thanks.

Apply SQL 9.3 typing rules for CASE-WHEN

7d6a66c

RCHowell requested a review from johnedquinn March 14, 2024 23:02

RCHowell added 2 commits March 15, 2024 09:00

Adds .flatten() and maintains union for structs

89ed22d

Inserts explicit CAST for branch coercions

35275b0

johnedquinn reviewed Mar 18, 2024

View reviewed changes

yliuuuu requested changes Mar 18, 2024

View reviewed changes

alancai98 reviewed Mar 18, 2024

View reviewed changes

RCHowell added 2 commits March 18, 2024 14:35

Address PR feedback and initialize CASE-WHEN tests

2e52735

Adds CASE-WHEN typer tests

eb8f47c

alancai98 reviewed Mar 18, 2024

View reviewed changes

Update partiql-planner/src/testFixtures/resources/inputs/basics/case.sql

aa604a7

Co-authored-by: Alan Cai <[email protected]>

alancai98 previously approved these changes Mar 18, 2024

View reviewed changes

alancai98 and others added 2 commits March 19, 2024 13:49

Prepare v0.14.5-SNAPSHOT (#1392)

d089e29

Adds tests for accumulate only MISSING

6bb0cb6

RCHowell dismissed alancai98’s stale review via 6bb0cb6 March 19, 2024 20:49

yliuuuu approved these changes Mar 19, 2024

View reviewed changes

RCHowell merged commit f42e74f into main Mar 19, 2024
10 checks passed

RCHowell deleted the case-when-typing branch March 19, 2024 21:02

RCHowell mentioned this pull request Mar 25, 2024

Updates CHANGELOG for CASE-WHEN #1401

Merged

alancai98 mentioned this pull request Mar 29, 2024

Add COALESCE and NULLIF to the logical plan #1404

Merged

RCHowell added a commit that referenced this pull request Apr 2, 2024

Apply SQL 9.3 typing rules for CASE-WHEN (#1391)

a327b20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply SQL 9.3 typing rules for CASE-WHEN #1391

Apply SQL 9.3 typing rules for CASE-WHEN #1391

RCHowell commented Mar 14, 2024 •

edited

Loading

github-actions bot commented Mar 14, 2024 •

edited

Loading

codecov-commenter commented Mar 15, 2024 •

edited

Loading

johnedquinn Mar 18, 2024

alancai98 Mar 18, 2024

RCHowell Mar 18, 2024 •

edited

Loading

yliuuuu left a comment

yliuuuu Mar 18, 2024

RCHowell Mar 18, 2024

yliuuuu Mar 19, 2024

alancai98 Mar 18, 2024

alancai98 Mar 18, 2024

RCHowell Mar 18, 2024

yliuuuu left a comment

		when (boolOrNull(condition.op)) {
		true -> return branch.rex // fold

Apply SQL 9.3 typing rules for CASE-WHEN #1391

Apply SQL 9.3 typing rules for CASE-WHEN #1391

Conversation

RCHowell commented Mar 14, 2024 • edited Loading

Relevant Issues

Description

Other Information

License Information

github-actions bot commented Mar 14, 2024 • edited Loading

Conformance comparison report

codecov-commenter commented Mar 15, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RCHowell Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

yliuuuu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yliuuuu left a comment

Choose a reason for hiding this comment

RCHowell commented Mar 14, 2024 •

edited

Loading

github-actions bot commented Mar 14, 2024 •

edited

Loading

codecov-commenter commented Mar 15, 2024 •

edited

Loading

RCHowell Mar 18, 2024 •

edited

Loading