Merges main into v1 #1481

johnedquinn · 2024-06-04T22:46:21Z

Description

Merges main into v1.
These changes notably contain the removal of NullType and MissingType. I'd mostly look at the final commit (cc3654a) to see what I needed to change.

Other Information

This is unreleased
Do your changes comply with the Contributing Guidelines
and Code Style Guidelines? NO

License Information

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

Removes all logic regarding absent types in planner

Adds TODO Simplifies typing of index operator Updates comment

…missing Deprecates absent types and removes associated planner logic

Updates planning tests Removes exhaustive attribute from dynamic call

alancai98

Reviewed just the last commit fixing the tests cc3654a (#1481)

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

partiql-planner/src/test/kotlin/org/partiql/planner/PlannerErrorReportingTests.kt

alancai98 · 2024-06-06T22:20:11Z

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

@@ -760,22 +760,19 @@ class PlanTyperTestsPorted {
            ErrorTestCase(
                name = "BITWISE_AND_MISSING_OPERAND",
                query = "1 & MISSING",
-                expected = unionOf(INT4, INT8, INT),
+                expected = ANY, // TODO: Is this unionOf(INT4, INT8, INT) ?


Hmm thought it should also be the union(INT4, INT8, INT) #1463. May need to check this again?

Agreed -- however, I'd like to just get this in. Realistically, if we want to narrow down the type, this can be accomplished using the "unknown" type that PostgreSQL exposes.

Could consider capturing the future addition of an "unknown" type in a GH issue. Since this is just a merge, we can punt on the behavior on this (and similar tests).

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

partiql-eval/src/test/kotlin/org/partiql/eval/internal/PartiQLEngineDefaultTest.kt

RCHowell · 2024-06-11T16:35:00Z

partiql-parser/build.gradle.kts

+    // TODO remove in next major version release.
+    rename("PartiQLParser.g4", "PartiQL.g4")


Note to self to remove the rename in the v1 branch.

ACK. I'm going to keep it since we know that we plan on merging this to main to perform a release before V1. This should be removed once all of our deprecation notices are eventually deleted.

RCHowell · 2024-06-11T16:40:55Z

partiql-planner/src/main/kotlin/org/partiql/planner/internal/ProblemGenerator.kt

+        rex(StaticType.ANY, rexOpMissing(problem, causes))

    fun missingRex(causes: Rex.Op, problem: Problem): Rex =
-        rex(StaticType.MISSING, rexOpMissing(problem, listOf(causes)))
+        rex(StaticType.ANY, rexOpMissing(problem, listOf(causes)))

    fun errorRex(causes: List<Rex.Op>, problem: Problem): Rex =
-        rex(StaticType.MISSING, rexOpErr(problem, causes))
+        rex(StaticType.ANY, rexOpErr(problem, causes))

    fun errorRex(trace: Rex.Op, problem: Problem): Rex =
-        rex(StaticType.MISSING, rexOpErr(problem, listOf(trace)))
+        rex(StaticType.ANY, rexOpErr(problem, listOf(trace)))


Out of curiosity, will these become UNKNOWN?

Hmm. Good question. I'm unsure. unknown is used for literal values whose types need to be inferred based on their context (and original value).

Just thinking out loud here, but the goal of the planner is to produce a plan that can be "almost" directly executed. Given our gradual nature, the execution must execute the same way as a fully typed plan would have. Therefore, if unknown is treated in such a way that it may be handled in function resolution, then I think it would be appropriate.

Maybe that is appropriate in some scenarios. But I'm also thinking about functions that always receive the missing value as one of its inputs. I imagine that, currently, this probably collapses as a problem to always return missing -- though, it should be typed as the original function's type.

alancai98

Merge and updated tests look good. Left a minor suggestion to track adding an unknown type since there are a few tests that will have a different result type.

alancai98 · 2024-06-11T20:19:10Z

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

@@ -760,22 +760,19 @@ class PlanTyperTestsPorted {
            ErrorTestCase(
                name = "BITWISE_AND_MISSING_OPERAND",
                query = "1 & MISSING",
-                expected = unionOf(INT4, INT8, INT),
+                expected = ANY, // TODO: Is this unionOf(INT4, INT8, INT) ?


Could consider capturing the future addition of an "unknown" type in a GH issue. Since this is just a merge, we can punt on the behavior on this (and similar tests).

partiql-eval/src/test/kotlin/org/partiql/eval/internal/PartiQLEngineDefaultTest.kt

partiql-planner/src/test/kotlin/org/partiql/planner/PlannerErrorReportingTests.kt

This reverts commit 20a8434.

This reverts commit 0e1564e.

johnedquinn · 2024-06-11T20:58:34Z

As this was pre-approved, and there are no changes between this revision and 145991e, I'm overriding and merging. The conformance test fix is actively being worked on by @alancai98 on the main branch and will be ported to v1 after.

johnedquinn and others added 10 commits May 16, 2024 11:34

Deprecates absent types

f41e12d

Removes all logic regarding absent types in planner

Addresses PR comments

4ae74d3

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

b82ac8c

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

8d5f1f9

Addresses PR Feedback

994635a

Adds TODO Simplifies typing of index operator Updates comment

Merge pull request partiql#1463 from johnedquinn/main-deprecate-null-…

8402db6

…missing Deprecates absent types and removes associated planner logic

Fixes ANTLR parser grammar and renames parser g4 file (partiql#1474)

a8c618a

Changes INT/INTEGER to be an alias for INT4 (partiql#1473)

a2d5398

Merge branch 'main' into v1

9ee2e59

Fixes tests

cc3654a

Updates planning tests Removes exhaustive attribute from dynamic call

alancai98 reviewed Jun 6, 2024

View reviewed changes

Addresses PR comments

145991e

RCHowell reviewed Jun 11, 2024

View reviewed changes

alancai98 approved these changes Jun 11, 2024

View reviewed changes

johnedquinn added 4 commits June 11, 2024 13:37

Updates Gradle to 8.7

20a8434

Updates conformance tests to Gradle 8.7

0e1564e

Revert "Updates Gradle to 8.7"

53f3502

This reverts commit 20a8434.

Revert "Updates conformance tests to Gradle 8.7"

a938485

This reverts commit 0e1564e.

johnedquinn merged commit 46fea3b into partiql:v1 Jun 11, 2024
6 checks passed

johnedquinn deleted the v1-merge-main branch June 11, 2024 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merges main into v1 #1481

Merges main into v1 #1481

johnedquinn commented Jun 4, 2024

alancai98 left a comment

alancai98 Jun 6, 2024

johnedquinn Jun 11, 2024

alancai98 Jun 11, 2024

RCHowell Jun 11, 2024

johnedquinn Jun 11, 2024

RCHowell Jun 11, 2024

johnedquinn Jun 11, 2024

alancai98 left a comment

alancai98 Jun 11, 2024

johnedquinn commented Jun 11, 2024

		// TODO remove in next major version release.
		rename("PartiQLParser.g4", "PartiQL.g4")

Merges main into v1 #1481

Merges main into v1 #1481

Conversation

johnedquinn commented Jun 4, 2024

Description

Other Information

License Information

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

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

johnedquinn commented Jun 11, 2024