feat: Support Spark expression seconds_of_time#3618
feat: Support Spark expression seconds_of_time#36180lai0 wants to merge 6 commits intoapache:mainfrom
Conversation
martin-g
changed the title
martin-g
changed the title
| } | ||
| } | ||
|
|
||
| def secondOfTimeToProto( |
There was a problem hiding this comment.
This method should probably be moved to datetime.scala instead.
There was a problem hiding this comment.
Thanks for the feedback. I'll make changes based on your comment.
| // Right child is the encoding expression. | ||
| stringDecode(expr, s.charset, s.bin, inputs, binding) | ||
|
|
||
| case _ if expr.getClass.getSimpleName == "SecondOfTime" => |
There was a problem hiding this comment.
- Using the simpleName could return
truefor a totally unrelated class - It seems you have not tested your suggested changes because the name of the class is SecondsOfTime (https://github.com/apache/spark/blob/b38e8eaece84e80687ab1f707859c1e5ee7e4c9f/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala#L354)
There was a problem hiding this comment.
Thanks for review. Sorry to miss 's', I will fix it in next commit.
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
| val timeZoneId = { | ||
| val exprClass = expr.getClass | ||
| try { | ||
| val timeZoneIdMethod = exprClass.getMethod("timeZoneId") |
There was a problem hiding this comment.
Could you please point me to the def timeZoneId() method or timeZoneId field in SecondsOfTime in Spark. I fail to find them in any of the parent classes/traits.
In addition maybe it would be good to add a cache and execute the reflection just once.
There was a problem hiding this comment.
I originally implemented this reflection to handle potential future scenarios where Spark might add a timeZoneId to SecondsOfTime. However, since this field doesn't exist in any currently supported Spark versions, it is effectively dead code. To reduce maintenance overhead, I’ll remove the reflection logic and explicitly default it to 'UTC', consistent with other datetime expressions.
2 participants
Which issue does this PR close?
Closes #3129
Rationale for this change
Comet previously did not support the Spark
SecondsOfTimeexpression, so queries using second(time_expr) could fall back to JVM execution instead of running natively.What changes are included in this PR?
This change adds a Serde handler for SecondsOfTime that reuses the existing Second protobuf and Rust implementation, ensuring consistent behavior while avoiding new native code paths.
How are these changes tested?
Added a Scala unit test SecondsOfTime expression support in CometExpressionSuite
./mvnw test -Dsuites="org.apache.comet.CometSqlFileTestSuite hour,org.apache.comet.CometExpressionSuite second" -Dtest=none./mvnw clean compile -DskipTests