Add COALESCE and NULLIF to the logical plan
#1404
Conversation
Conformance comparison report
Number passing in both: 5384 Number failing in both: 434 Number passing in Base (6fbfa78) but now fail: 0 Number failing in Base (6fbfa78) but now pass: 0 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1404 +/- ##
=========================================
Coverage 73.14% 73.14%
Complexity 2393 2393
=========================================
Files 247 247
Lines 17623 17623
Branches 3176 3176
=========================================
Hits 12890 12890
Misses 3856 3856
Partials 877 877
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
alancai98
force-pushed
the
add-coalesce-nullif-to-plan
branch
from
b178180
to
808bbf7
Compare
partiql-planner/src/main/kotlin/org/partiql/planner/internal/transforms/RexConverter.kt
Show resolved
Hide resolved
| val v2 = visitRex(node.v2, node.v2.type) | ||
| val typer = DynamicTyper() | ||
| typer.accumulate(NULL) | ||
| typer.accumulate(v1.type) |
There was a problem hiding this comment.
Only add v1's type here. I found that posgresql actually does something different here. Per their docs:
The result has the same type as the first argument — but there is a subtlety. What is actually returned is the first argument of the implied = operator, and in some cases that will have been promoted to match the second argument's type. For example, NULLIF(1, 2.2) yields numeric, because there is no integer = numeric operator, only numeric = numeric.
Per my read of SQL-99, I can't find any mention of this promotion behavior, so leaving the result type as just the minimal common supertype of (<type v1>, null). Postgresql would do something like the minimal common supertype of (<type v1>, <type v2>, null).
There was a problem hiding this comment.
I believe we can skip the dynamic typer here. It's just type of union(v1, null).flatten() or something as simple as that
There was a problem hiding this comment.
Yeah I'll simplify it. I initially included the dynamic typer in case we wanted to follow what postgresql did, which is easy to do w/ the dynamic typer.
There was a problem hiding this comment.
Followup from offline discussion, will keep the dynamic typer here to ensure consistency with the typing of CASE WHEN and COALESCE.
partiql-planner/src/main/kotlin/org/partiql/planner/internal/transforms/RexConverter.kt
Show resolved
Hide resolved
partiql-planner/src/main/kotlin/org/partiql/planner/internal/transforms/RexConverter.kt
Outdated
Show resolved
Hide resolved
| val v2 = visitRex(node.v2, node.v2.type) | ||
| val typer = DynamicTyper() | ||
| typer.accumulate(NULL) | ||
| typer.accumulate(v1.type) |
There was a problem hiding this comment.
I believe we can skip the dynamic typer here. It's just type of union(v1, null).flatten() or something as simple as that
Relevant Issues
Related to partiql/partiql-scribe#26.
Description
Previous rewrite according to SQL-99 spec was creating large logical plans with nested
COALESCEandNULLIFexpressions. This change makes dedicatedCOALESCEandNULLIFnodes within the logical plan.Other than that,
COALESCE(context in AddCOALESCEandNULLIFto the logical plan #1404 (comment))StaticType.ANY, where it was previously getting expanded to all of its typesOther Information
Updated Unreleased Section in CHANGELOG: [YES]
Any backward-incompatible changes? [YES]
Adding new nodes to the plan is technically a breaking change for the
PlanVisitorinterface. Tracking the sprout generation to add a warning on the interface as a separate issue -- #1405.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.