feat: Implement TimeType support - Infrastructure - sort, min/max#4951
feat: Implement TimeType support - Infrastructure - sort, min/max#4951YutaLin wants to merge 6 commits into
Conversation
628b715 to
968c304
Compare
|
Hi @parthchandra, can you help me review this? Thanks! |
parthchandra
left a comment
There was a problem hiding this comment.
Do we need to handle Literal Time64(Nanosecond) in the planner (create_expr)?
Maybe try a query with ORDER BY time_col LIMIT 10
# Conflicts: # spark/src/main/scala/org/apache/comet/serde/aggregates.scala
Hi @parthchandra, thanks for review! CometLiteral already serializes a non-null TimeType literal as LongVal, but create_expr only handled LongVal for Int64 and timestamps. I added the missing conversion to ScalarValue::Time64Nanosecond(Some(value)) and a focused Rust unit test. |
parthchandra
left a comment
There was a problem hiding this comment.
@YutaLin could you rebase? I have one optional comment, otherwise this is looking good.
| ) | ||
| SORT BY t ASC NULLS LAST, id DESC | ||
|
|
||
| query |
There was a problem hiding this comment.
Optional: a test like -
query
SELECT id, t
FROM (
SELECT id, make_time(hours, minutes, secs) AS t
FROM test_make_time_sort
)
ORDER BY t ASC NULLS FIRST
LIMIT 3
would be an additional check for supportedScalarSortElementType but reached via a different path.
Which issue does this PR close?
Closes #4379
Rationale for this change
Support sort, min/max for TimeType, include native and JVM columnar
What changes are included in this PR?
support sort and add sql tests.
How are these changes tested?
run sql test in spark4.1, spark4.2