Adds support for nullable casts and missable/missingCall functions #1422
Conversation
| @@ -584,18 +584,21 @@ internal class PlanTyper( | |||
|
|
|||
| // Type the arguments | |||
There was a problem hiding this comment.
I'm curious if https://github.com/partiql/partiql-lang-kotlin/blob/v1/partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/PlanTyper.kt#L1357-L1405 can be a replacement for this? This is a lot of logic that isn't in V1. Ideally we can bring the latest to 0.14.+
There was a problem hiding this comment.
+1 to Robert's question. Can the same logic from v1 be ported to this PR/main? Have a couple concerns that
- the PR may have a different typing behavior from the
v1branch - the code changes may make it tricky to merge
mainintov1(orv1intomain)
partiql-planner/src/main/kotlin/org/partiql/planner/internal/typer/TypeLattice.kt
Show resolved
Hide resolved
partiql-planner/src/test/kotlin/org/partiql/planner/internal/typer/operator/OpConcatTest.kt
Show resolved
Hide resolved
| @@ -14,13 +14,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| ### Deprecated | |||
|
|
|||
| ### Fixed | |||
| - Fixes [#1398](https://github.com/partiql/partiql-lang-kotlin/issues/1398) by handling "missable" arguments to | |||
There was a problem hiding this comment.
nit: changelog entry added to the template and not the Unreleased section below
| @@ -57,6 +60,8 @@ public sealed class FunctionSignature( | |||
| isNullable: Boolean = true, | |||
| @JvmField public val isDeterministic: Boolean = true, | |||
| @JvmField public val isNullCall: Boolean = false, | |||
| @JvmField public val isMissable: Boolean = false, | |||
There was a problem hiding this comment.
Any reason isMissable on the main branch defaults to false? FnSignature on the v1 branch defaults to true:
There was a problem hiding this comment.
A little confused w/ the divergence in behavior. Seems like all the functions defined in the PartiQLHeader are all not "missible"?
| * @property isMissable Indicates whether the function's operator may return MISSING. | ||
| * @property isMissingCall Indicates behavior when any of the arguments are MISSING. If set to true, the function | ||
| * will return MISSING. If false, the operator itself will handle the MISSING. |
There was a problem hiding this comment.
nit: for easier merging, may be helpful to use the same kdoc comments from the FnSignature defined on v1 branch:
| @@ -182,15 +184,24 @@ internal object PartiQLHeader : Header() { | |||
| unary("neg", t, t) | |||
| } | |||
|
|
|||
| private fun eq(): List<FunctionSignature.Scalar> = types.all.map { t -> | |||
| private fun eq(): List<FunctionSignature.Scalar> = types.all.filterNot { it == ANY || it == MISSING }.map { t -> | |||
There was a problem hiding this comment.
There's a divergence here between the modeling of functions between this PR and the v1 branch. E.g.
- For
eq, thev1branch includes all the types and they have the sameisNullable,isMissingCall`, etc. configurations - For the logical ops, the
v1branch uses the defaults forisMissable = trueandisMissingCall = true, whereas this PR definesisMissable = falseandisMissingCall = false
I'm curious the reasoning for the modeling differences.
| @@ -584,18 +584,21 @@ internal class PlanTyper( | |||
|
|
|||
| // Type the arguments | |||
There was a problem hiding this comment.
+1 to Robert's question. Can the same logic from v1 be ported to this PR/main? Have a couple concerns that
- the PR may have a different typing behavior from the
v1branch - the code changes may make it tricky to merge
mainintov1(orv1intomain)
|
Closing as an alternative solution is being actively pursued. |
Relevant Issues
Description
v1largely has these changes already, this is a port of its functionality.Other Information
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.