feat: Support Spark expression seconds_of_time#3618
feat: Support Spark expression seconds_of_time#36180lai0 wants to merge 13 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>
…scala Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
…scala Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
…scala Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
|
Thanks @martin-g for pointing that out. |
|
Hi @andygrove , could you please take a look when your schedule permits? Thanks : ) |
|
@0lai0 could you fix the compilation issue. thx. |
|
Sure, thank for running CI test. I'll fix it right away. |
|
SecondsOfTime is new in Spark 4.1.0 as far as I know, and Comet does not support Spark 4.1.0 yet |
|
Thanks for pointing that out! I wasn't aware that SecondsOfTime is only available in Spark 4.1.0. Since Comet doesn't support Spark 4.1 yet, I'll put this PR on hold until that support is added. Should I close this PR for now, or keep it open as a draft? |
@0lai0 Comet now supports Spark 4.0 (and 4.1) |
|
@0lai0 I moved to draft. Please mark as ready for review when you have time to work on this again. |
3 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