Merges tag v0.14.8 into v1 #1565
Conversation
Picks commits from main to include in 0.14.6
… typer (partiql#1551) * preserve constraint for decimal, string in dynamic typer
| @@ -23,7 +23,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| Thank you to all who have contributed! | |||
| --> | |||
|
|
|||
| ## [Unreleased] | |||
| ## [0.14.8] | |||
There was a problem hiding this comment.
The v0.14.8 CHANGELOG entry does not match w/ the v0.14.8 branch's CHANGELOG -- https://github.com/partiql/partiql-lang-kotlin/blob/v0.14.8/CHANGELOG.md. Perhaps it's because of a prior merge of main to v1?
There was a problem hiding this comment.
Nice catch. Somehow that merged cleanly.
There was a problem hiding this comment.
Seems like we're also missing some of the CHANGELOG release links for v0.14.6-v0.14.8 -- https://github.com/johnedquinn/partiql-lang-jvm/blob/v1-merge-v0_14_8/CHANGELOG.md?plain=1#L1124-L1125.
partiql-eval/src/test/kotlin/org/partiql/eval/internal/PartiQLEngineDefaultTest.kt
Outdated
Show resolved
Hide resolved
| @@ -132,6 +146,47 @@ internal class DynamicTyper { | |||
| return s to mapping | |||
| } | |||
|
|
|||
| private fun computeDecimal(): CompilerType? { | |||
There was a problem hiding this comment.
Wonder if we should preserve the change from Yingtao's comment in commit 873b6c4 (#1554). Basically,
- Make it clearer that this function is only for computing constrained decimals. So rename to
computeConstrainedDecimal. - Error when we receive an arbitrary decimal rather than returning
null, since that would indicate a bug in the dynamic typer. This function shouldn't be called when we have previously encountered an arbitrary decimal.
There was a problem hiding this comment.
The merged change is still wrong. The supertype of INT, SMALLINT, and DECIMAL(5, 0), according to #1554 is DECIMAL. Though, when computeDecimal is invoked to compute the precision and scale, it throws an error when folding over the types of INT and SMALLINT.
This PR merges the not completely correct code while mitigating the thrown exception. I tried to apply as minimal changes as possible to the merge, but this is one area known to have a bug.
There was a problem hiding this comment.
Ok -- then if the merged behavior is misaligned w/ the change in 0.14.8, then we should either
- track it in a TODO and tracking issue
- or fix it in this PR if it's not too much work
Adds DynamicTyper issue link
Description
v0.14.8intov1License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.