[SPARK-29819][SQL] Introduce an enum for interval units #26455
Conversation
|
@srowen @dongjoon-hyun @cloud-fan Does this make sense for you? |
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
Show resolved
Hide resolved
| object IntervalUnit extends Enumeration { | ||
| type IntervalUnit = Value | ||
|
|
||
| val Nanosecond = Value(0, "nanosecond") |
There was a problem hiding this comment.
What is the first value (here 0) used for?
There was a problem hiding this comment.
This is just be sure that enum values are ordered, so Microsecond < ... < Second < ... < Year.
| @@ -28,6 +28,22 @@ import org.apache.spark.unsafe.types.{CalendarInterval, UTF8String} | |||
|
|
|||
| object IntervalUtils { | |||
|
|
|||
| object IntervalUnit extends Enumeration { | |||
There was a problem hiding this comment.
This is intended to be internal only?
|
Thank you for pinging me, @MaxGekk . |
| val Day = Value(6, "day") | ||
| val Week = Value(7, "week") | ||
| val Month = Value(8, "month") | ||
| val Year = Value(9, "year") |
There was a problem hiding this comment.
Could you use all capitals like YEAR please, @MaxGekk ?
There was a problem hiding this comment.
This contradicts to the Scala coding style guide: https://github.com/databricks/scala-style-guide#naming-convention Enums should be PascalCase.
There was a problem hiding this comment.
I knew it already, but it's not in Apache Spark. That's the reason I ask like this. #26455 (comment) .
|
@MaxGekk . The enum convention is a little mixed in Apache Spark, but all capitals are dominant in |
|
Test build #113503 has finished for PR 26455 at commit
|
|
Test build #113527 has finished for PR 26455 at commit
|
|
jenkins, retest this, please |
|
Test build #113531 has finished for PR 26455 at commit
|
What changes were proposed in this pull request?
In the PR, I propose an enumeration for interval units with the value
YEAR,MONTH,WEEK,DAY,HOUR,MINUTE,SECOND,MILLISECOND,MICROSECONDandNANOSECOND.Why are the changes needed?
Does this PR introduce any user-facing change?
No
How was this patch tested?
By existing test suites
ExpressionParserSuiteandIntervalUtilsSuite