Removes sexp and symbol from PType and Datum #1633
Conversation
johnedquinn
force-pushed
the
v1-remove-ion
branch
4 times, most recently
from
0ef2c89
to
a6b7dd8
Compare
johnedquinn
force-pushed
the
v1-remove-ion
branch
from
a6b7dd8
to
6d5cb42
Compare
| @@ -173,7 +172,7 @@ internal class RelOpExclude( | |||
| // apply exclusions to subtree | |||
| var value = element | |||
| // apply collection index exclusions at deeper levels for lists and sexps | |||
There was a problem hiding this comment.
Can remove reference to sexps here
| // apply collection index exclusions at deeper levels for lists and sexps | |
| // apply collection index exclusions at deeper levels for lists |
| } | ||
|
|
||
| IonType.STRING -> { | ||
| IonType.STRING, IonType.SYMBOL -> { |
There was a problem hiding this comment.
For the PartiQLValueIonReader changes, what's the intended behavior for Ion types that we are not representing in PartiQL? Seems like this PR coerces SYMBOL to PartiQL string values and SEXPs to PartiQL list values. Should we give an error instead?
| @@ -70,16 +70,17 @@ public enum class PartiQLValueType { | |||
| ) | |||
| public fun toPType(): PType { | |||
| return when (this) { | |||
| DECIMAL, DECIMAL_ARBITRARY -> PType.decimal() | |||
| DECIMAL -> PType.decimal() | |||
| DECIMAL_ARBITRARY -> PType.decimal() // TODO: Figure out precision/scale implications. | |||
There was a problem hiding this comment.
Should decimal_arb also give an error like you do for symbol and sexp?
| @@ -89,8 +89,8 @@ internal val Fn_PLUS__DECIMAL_ARBITRARY_DECIMAL_ARBITRARY__DECIMAL_ARBITRARY = F | |||
| name = "plus", | |||
| returns = PType.decimal(), | |||
| parameters = arrayOf( | |||
| @Suppress("DEPRECATION") Parameter("lhs", PType.decimal()), | |||
| @Suppress("DEPRECATION") Parameter("rhs", PType.decimal()), | |||
| Parameter("lhs", PType.decimal()), | |||
There was a problem hiding this comment.
FN name var for above uses DECIMAL_ARBITRARY, perhaps needs to get changed?
| @@ -88,8 +88,8 @@ internal val Fn_LTE__DECIMAL_ARBITRARY_DECIMAL_ARBITRARY__BOOL = Function.static | |||
| name = "lte", | |||
| returns = PType.bool(), | |||
| parameters = arrayOf( | |||
| @Suppress("DEPRECATION") Parameter("lhs", PType.decimal()), | |||
| @Suppress("DEPRECATION") Parameter("rhs", PType.decimal()), | |||
| Parameter("lhs", PType.decimal()), | |||
There was a problem hiding this comment.
Similarly, fn name variable uses DECIMAL_ARB. Perhaps should get changed?
Also applies for
- lt
- gt
- gte
- between
- may have some others, so probably need to do a check on Builtins.kt
| name = "concat", | ||
| returns = PType.string(), | ||
| returns = PType.character(256), // TODO: Handle length |
There was a problem hiding this comment.
Should these TODOs for length be handled in this PR or tracked somewhere else?
| PType.time(6), // TODO: Precision | ||
| PType.timez(6), // TODO: Precision | ||
| ) | ||
|
|
||
| val allTimeStampPType = setOf( | ||
| PType.timestamp(6), // TODO: Precision | ||
| PType.timestampz(6), // TODO: Precision | ||
| ) | ||
|
|
||
| val allDateTimePType = allDatePType + allTimePType + allTimeStampPType | ||
|
|
||
| val allCharStringPType = setOf( | ||
| PType.character(256), // TODO: Length | ||
| PType.varchar(256), // TODO: Length | ||
| PType.string(), | ||
| PType.clob(Int.MAX_VALUE), // TODO: Length | ||
| ) | ||
|
|
||
| val allBinaryPType = setOf( | ||
| PType.blob(Int.MAX_VALUE), // TODO: Length |
There was a problem hiding this comment.
Will these TODOs for precision/length be addressed in this PR or are they tracked in some other issue?
Description
Testing
Follow-ups
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.