fix: Refactor arithmetic serde and fix correctness issues with EvalMode::TRY #2018
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2018 +/- ##
============================================
+ Coverage 56.12% 58.63% +2.51%
- Complexity 976 1237 +261
============================================
Files 119 135 +16
Lines 11743 13065 +1322
Branches 2251 2382 +131
============================================
+ Hits 6591 7661 +1070
- Misses 4012 4179 +167
- Partials 1140 1225 +85 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
andygrove
changed the title
|
@rishvin, This PR is still a draft, but could you review the changes to the |
Thanks @andygrove for the heads up. I took the look for |
| def supportedDataType(dt: DataType): Boolean = dt match { | ||
| case _: ByteType | _: ShortType | _: IntegerType | _: LongType | _: FloatType | | ||
| _: DoubleType | _: StringType | _: BinaryType | _: TimestampType | _: TimestampNTZType | | ||
| _: DecimalType | _: DateType | _: BooleanType | _: NullType => | ||
| true | ||
| case _ => | ||
| false | ||
| } |
There was a problem hiding this comment.
This is copied over from QueryPlanSerde. I removed the complex types, but I am sure we could remove more types in the future.
| private def isPrimitive(expression: Expression): Boolean = expression.dataType match { | ||
| case _: ByteType | _: ShortType | _: IntegerType | _: LongType | _: FloatType | | ||
| _: DoubleType | _: TimestampType | _: DateType | _: BooleanType | _: DecimalType => | ||
| true | ||
| case _ => false | ||
| } |
There was a problem hiding this comment.
This is moved without modification from QueryPlanSerde. I am not sure why this method is needed. I'm pretty sure that we can only perform arithmetic on primitive types?
There was a problem hiding this comment.
I'm not sure why this is needed either, but while we are checking, should this also check for TimestampNTZ?
| private def isPrimitive(expression: Expression): Boolean = expression.dataType match { | ||
| case _: ByteType | _: ShortType | _: IntegerType | _: LongType | _: FloatType | | ||
| _: DoubleType | _: TimestampType | _: DateType | _: BooleanType | _: DecimalType => | ||
| true | ||
| case _ => false | ||
| } |
There was a problem hiding this comment.
I'm not sure why this is needed either, but while we are checking, should this also check for TimestampNTZ?
Which issue does this PR close?
Part of #2019
Partial fix for #2021
Rationale for this change
See #2019
What changes are included in this PR?
Refactor the following expressions:
Functional changes:
supportedDataTypefor math expressions (we no longer claim to support performing math operations on complex types, for example)EvalMode:TRYbecause we haven't implemented support for that in native code yet and produce incorrect results in some casesHow are these changes tested?