Deprecates absent types and removes associated planner logic #1463

johnedquinn · 2024-05-15T23:24:26Z

Description

As discussed with the rest of the PartiQL maintainers, the null and missing values have leaked into our type semantics -- causing issues when it comes to function resolution, unsafe casts, and dynamic functions.
This PR deprecates NullType and MissingType and removes their usage in the planner. The implication is that ALL types may contain the null and missing values. See the CHANGELOG for all deprecated APIs.
In this PR, literal NULL and MISSING values are typed as ANY (dynamic). In the future, we may want to pursue the "unknown" type that PostgreSQL internally uses.
Smaller changes include:
- I deprecated AnyOfType's primary constructor. We've been plagued for too long by forgetting to use .flatten() .
- The majority of this diff is me getting rid of "null" and "missing" in the testFixtures.

Other Information

Updated Unreleased Section in CHANGELOG: YES
Any backward-incompatible changes? YES
- Not an API breaking change, but a behavioral one.
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.

johnedquinn · 2024-05-16T17:57:27Z

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

@@ -2144,15 +2136,13 @@ class PlanTyperTestsPorted {
                    ) FROM <<
                        { 'a': { 'b': 1 } },
                        { 'a': { 'b': 'hello' } },
-                        { 'a': NULL },


I updated this only because the literal NULL's type is unknown. Which leads to the inferred type to be ANY when used as an argument to a literal struct.

Not too sure why the empty struct in the input binding tuples doesn't cause ANY for the type. Perhaps that binding tuple is filtered out?

Not sure if there's a test showing this but if there's not, could still be helpful to include a test showing that the inferred type for a path expression on a literal struct with a NULL/MISSING value is ANY. E.g. { 'a': NULL }.a -> ANY and { 'a': MISSING }.a -> ANY

johnedquinn · 2024-05-16T17:58:38Z

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

-                    )
-                )
-            ),
-            SuccessTestCase(


This is deleted now since ALL types are nullable.

Removes all logic regarding absent types in planner

alancai98

nice work in removing the absent types from the static type modeling. I think it helps simplify a lot of the typing and test fixture involving typing logic quite a bit. comments were mostly minor though I did have some questions about some behavioral changes in tests.

alancai98 · 2024-05-16T19:06:20Z

CHANGELOG.md

@@ -28,8 +28,25 @@ Thank you to all who have contributed!
 ### Added

 ### Changed
+- **Behavioral change**: The planner now does NOT support the NullType and MissingType variants of StaticType. The logic


Something for us to consider when releasing this change is that it's a pretty big behavioral change w/ the typing of plans. We should consider cutting a new major version release rather than a minor version release.

I'm concerned about this as well. We have many customers who assert on current typing judgements, we can't make any typing changes without breaking them.

plugins/partiql-local/src/main/kotlin/org/partiql/plugins/local/LocalSchema.kt

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

partiql-types/src/main/kotlin/org/partiql/types/StaticType.kt

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

alancai98 · 2024-05-16T22:39:43Z

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

@@ -635,26 +637,23 @@ class PlanTyperTestsPorted {
            SuccessTestCase(
                name = "BITWISE_AND_NULL_OPERAND",
                query = "1 & NULL",
-                expected = StaticType.NULL,
+                expected = unionOf(INT4, INT8, INT),


Hmm, curious why this will now give a union of these int types? Similarly for the below case w/ MISSING.

Discussed offline -- but it has to do with the fact that a literal NULL has an unknown type, which gets interpreted as ANY for our type inference. Therefore, the applicable functions for bitwise_and are only for the integer types that correspond to the LHS being an int_32.

Before this PR, 1 & NULL would be constant folded in the typer, which is wrong, because it doesn't actually give insight to the type being returned (just the value). This change narrows down the type. Though, in the future (or, if you'd like in this PR), we can attempt to find the highest precedence function when the argument is the literal NULL/MISSING value. This is kind of what PostgreSQL does. As it would result in more in-depth logic changes, I'd prefer a follow-up to this PR.

I see. Thanks for providing some context. Just to clarify the difference of behavior, previously, we would always collapse the types to the unknown type. For example:

CASE WHEN ... THEN NULL END -> type null

t_int32 & NULL -> type null

But since we get rid of the unknown types in this PR, the behavior the above would get changed to:

CASE WHEN ... THEN NULL END -> ANY -- only know that NULL gets returned but don't know anything about the type being returned

t_int32 & NULL -> union(int32, int64, int) -- only know that NULL gets returned and we have some information on the type being returned based on the resolvable functions

I'm still wondering if for the second case (t_int32 & NULL) we should still declare the type as NULL though. Consider the following valid query:

(t_int32 & NULL) || t_string

This would previously type the LHS of the concat to type null causing the entire expression to have an output type of null.

However with this PR, since the LHS of the concat would have type union(int32, int64, int) and the RHS of the concat would have type string, this results in an unresolved function for concat(<union(int4, int8, int)>, <string>).

Perhaps we shouldn't narrow the type further if we know an expression always returns NULL/MISSING and just output ANY?

Assuming concatenation of non-textual types isn't allowed, then the unresolved function is the expected behavior. The current behavior of returning the NULL type is a bug.

I guess what I'm saying here is that the null value is always typed when it comes to static analysis. The only scenario in which the type is unknown is when the literal NULL/MISSING keywords are used. From SQL:1999:

Since the null value is in every data type, the data type of the null value implied by the
keyword NULL cannot be inferred; hence NULL can be used to denote the null value only in
certain contexts, rather than everywhere that a literal is permitted.

To get around this, PostgreSQL represents this scenario with a distinct unknown type. They temporarily mark the null literal as unknown, and when it is input into a function, the type is narrowed to a specific type. We currently do not have the concept of "unknown", though, due to our dynamic nature, ANY gets us very close.

So, consider your example: (t_int32 & NULL) || t_string. In PostgreSQL, NULL would be typed as unknown. When using the binary and operator, the type is clarified and is labeled as an integer. Therefore, the signature looks like: & (int32, int32) -> int32. Then, the input to the concatenation is int32 and varchar. Since PostgreSQL allows for implicit coercion of int to text, it gets resolved.

While I'm in favor of using the unknown type in the future, ANY works as follows: typing the literal NULL results in ANY. The binary and operator has several relevant overloads, namely resulting in int32, int64, int. Therefore, with dynamic dispatch, those three types are the only types able to be returned. Assuming we do not allow concatenation of non-textual types, then we have enough knowledge to fail compilation and return the unresolved function representing concatenation.

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

RCHowell · 2024-05-21T22:08:57Z

CHANGELOG.md

@@ -28,8 +28,25 @@ Thank you to all who have contributed!
 ### Added

 ### Changed
+- **Behavioral change**: The planner now does NOT support the NullType and MissingType variants of StaticType. The logic


I'm concerned about this as well. We have many customers who assert on current typing judgements, we can't make any typing changes without breaking them.

partiql-planner/src/main/kotlin/org/partiql/planner/internal/transforms/PlanTransform.kt

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

RCHowell · 2024-05-21T22:35:59Z

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

-            val schema = when (node.type) {
-                Rel.Op.Join.Type.INNER -> l + r
-                Rel.Op.Join.Type.LEFT -> l + r.pad()
-                Rel.Op.Join.Type.RIGHT -> l.pad() + r
-                Rel.Op.Join.Type.FULL -> l.pad() + r.pad()
-            }


Why are you able to remove the padding?

All types are considered nullable, therefore, there isn't a need to pad the types with the NULL type.

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

RCHowell · 2024-05-21T22:46:35Z

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

+                handleAlwaysMissing()
+                return rex(ANY, rexOpPathKey(root, key))


Shouldn't we be returning a rex with rexOpMissing?

Yes, in the v1 branch we can do that. Main doesn't have that concept yet.

RCHowell · 2024-05-21T22:47:13Z

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

+            // Check Root Type
+            if (!root.type.mayBeType<StructType>()) {
+                handleAlwaysMissing()
+                return rex(ANY, rexOpErr("Symbol lookup may only occur on structs, not ${root.type}."))


Similarly we should have the rexOpMissing here — which is effectively an error node that we handle at runtime based upon evaluation mode.

#1463 (comment)

RCHowell · 2024-05-21T22:47:35Z

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

+                    handleAlwaysMissing()
+                    return rex(ANY, Rex.Op.Path.Symbol(root, node.key))


Missing node again

#1463 (comment)

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

Adds TODO Simplifies typing of index operator Updates comment

johnedquinn force-pushed the main-deprecate-null-missing branch from 2154f45 to 54c043f Compare May 16, 2024 17:17

johnedquinn commented May 16, 2024

View reviewed changes

johnedquinn force-pushed the main-deprecate-null-missing branch 3 times, most recently from 27e5a55 to d56ed09 Compare May 16, 2024 18:30

Deprecates absent types

f41e12d

Removes all logic regarding absent types in planner

johnedquinn force-pushed the main-deprecate-null-missing branch from d56ed09 to f41e12d Compare May 16, 2024 18:34

johnedquinn requested a review from alancai98 May 16, 2024 18:35

johnedquinn marked this pull request as ready for review May 16, 2024 18:35

alancai98 reviewed May 16, 2024

View reviewed changes

johnedquinn mentioned this pull request May 17, 2024

Remove typer's exclude last step optional logic #1468

Open

Addresses PR comments

4ae74d3

This was referenced May 17, 2024

Merges main into V1 #1469

Merged

Initializes Datum and adds functionality to evaluator #1451

Merged

johnedquinn added 2 commits May 20, 2024 11:14

Per PR feedback, removes NULL/MISSING from StaticType.ALL_TYPES

b82ac8c

Per PR feedback, removes NULL/MISSING from Ion-encoded typing tests

8d5f1f9

johnedquinn requested a review from RCHowell May 20, 2024 22:40

RCHowell requested changes May 21, 2024

View reviewed changes

Addresses PR Feedback

994635a

Adds TODO Simplifies typing of index operator Updates comment

johnedquinn requested a review from RCHowell May 23, 2024 00:09

RCHowell approved these changes May 23, 2024

View reviewed changes

johnedquinn merged commit 8402db6 into partiql:main May 28, 2024
7 checks passed

johnedquinn deleted the main-deprecate-null-missing branch May 28, 2024 18:23

This was referenced Jun 4, 2024

Fix Trino transpilation of nullable single field ROW generated from EXCLUDE partiql/partiql-scribe#46

Merged

Merges main into v1 #1481

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecates absent types and removes associated planner logic #1463

Deprecates absent types and removes associated planner logic #1463

johnedquinn commented May 15, 2024 •

edited

Loading

johnedquinn May 16, 2024

alancai98 May 18, 2024

alancai98 May 18, 2024

johnedquinn May 16, 2024

alancai98 left a comment

alancai98 May 16, 2024

RCHowell May 21, 2024

alancai98 May 16, 2024

johnedquinn May 17, 2024

alancai98 May 17, 2024 •

edited

Loading

johnedquinn May 20, 2024

johnedquinn May 20, 2024

RCHowell May 21, 2024

RCHowell May 21, 2024

johnedquinn May 22, 2024

RCHowell May 21, 2024

johnedquinn May 22, 2024

RCHowell May 21, 2024

johnedquinn May 22, 2024

RCHowell May 21, 2024

johnedquinn May 22, 2024

		handleAlwaysMissing()
		return rex(ANY, rexOpPathKey(root, key))

		handleAlwaysMissing()
		return rex(ANY, Rex.Op.Path.Symbol(root, node.key))

Deprecates absent types and removes associated planner logic #1463

Deprecates absent types and removes associated planner logic #1463

Conversation

johnedquinn commented May 15, 2024 • edited Loading

Description

Other Information

License Information

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

alancai98 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

alancai98 May 17, 2024 • edited Loading

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

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

johnedquinn commented May 15, 2024 •

edited

Loading

alancai98 May 17, 2024 •

edited

Loading