Add DayTimeIntervalType/YearMonthIntervalType support #3460
Conversation
| import org.apache.spark.sql.types.{DataType, DayTimeIntervalType, YearMonthIntervalType} | ||
|
|
||
| /** TypeSig for Spark 3.2.0+ which adds DayTimeIntervalType and YearMonthIntervalType support */ | ||
| final class TypeSig320( |
There was a problem hiding this comment.
I see two possibilities on how to do this
- We do something like this were we have a TypeSigBase in the common code and then an actual TypeSig implementation class in shim layers. The problem with this is
TypeSig.alland the other Spark helper values will not reflect "all" of the types. To do that we would need to move all or part of object TypeSig into the shim layer, or we would have to have the overrides for each of the things that touchTypeSig.alletc go into the shim layer. - We turn it around and have a static TypeSigUtil object in the shim layer that TypeSig calls into to do checks with new types.
For me I am leaning towards the second approach because it is consistent between the static and non-static code.
There was a problem hiding this comment.
Thx @revans2, I just had a new commit for the second approach. Could you help to take a look at it? Thx
|
build |
| @@ -93,4 +93,6 @@ trait Spark30XShims extends SparkShims { | |||
| } | |||
|
|
|||
| override def shouldFailDivOverflow(): Boolean = false | |||
|
|
|||
| override def getTypeSigUtil(): TypeSigUtil = TypeSigUtilUntil320 | |||
There was a problem hiding this comment.
We don't need anything in the Shim layer to make this happen. You can just have a single class named TypeSigUtil. Then you have different versions in the appropriate directories.
There was a problem hiding this comment.
Yeah, You're correct. Thx. Done
| } | ||
| } | ||
|
|
||
| override def getCastChecksAndSigs( |
There was a problem hiding this comment.
I think I would prefer it if we found a way to put daytimeChecks, sparkDaytimeSig yearmonthChecks and sparkYearmonthSig inside of the CastChecks class. This is just for consistency with where the rest of the checks are, and how they are overridden.
There was a problem hiding this comment.
Done. I just map the dataType to TypeEnum and invoke the existing function which accepts TypeEnum can resolve this issue.
Signed-off-by: Bobby Wang <[email protected]>
Signed-off-by: Bobby Wang <[email protected]>
Signed-off-by: Bobby Wang <[email protected]>
|
build |
|
build |
| @@ -115,6 +157,8 @@ object TypeEnum extends Enumeration { | |||
| val MAP: Value = Value | |||
| val STRUCT: Value = Value | |||
| val UDT: Value = Value | |||
| val DAYTIME: Value = Value | |||
| val YEARMONTH: Value = Value | |||
| } | |||
There was a problem hiding this comment.
Can we util ShimLoader.getSparkVersion to construct static version to set(TypeEnum) map?
Not know if feasible, if ok, just not need the spark 320 code.
There was a problem hiding this comment.
Adding more enums values dynamically is not something that scala supports as far as I can tell.
This PR tries to add DayTimeIntervalType and YearMonthIntervalType for TypeSig. To fix part of #3415
This PR introduces a new TypeSig320 which extends TypeSig, and overrides some methods which has data type checks.
The expressions which supports DayTimeIntervalType and YearMonthIntervalType need to be moved to Spark 320 shims to add TypeSig.DAYTIME and TypeSig.YEARMONTH into the checks.
@revans2, Could you give me some comments on this solution?