felipepessoto commented on PR #12967: URL: https://github.com/apache/gluten/pull/12967#issuecomment-5589139525
> Thanks @felipepessoto. Could you please clarify whether this implementation is based on the aggregate implementation for the `Timestamp` type in Velox, and whether it can be fully reused for the `TimestampNTZ` type? Yes, for `min` and `max`, this PR reuses Velox's existing timestamp aggregate implementation; it does not add a separate `TIMESTAMP_NTZ` aggregate kernel ([code](https://github.com/facebookincubator/velox/blob/68c4501e9ae33a2d6041cc3f9ce40e0bec376fc7/velox/functions/lib/aggregates/MinMaxAggregateBase.cpp#L613-L630)). Velox represents Spark `TIMESTAMP_NTZ` as `TIMESTAMP_UTC`. `TimestampUtcType` derives from `TimestampType`, so it has the same `TypeKind::TIMESTAMP` and native `Timestamp` value representation, while retaining distinct logical type identity and no session-timezone adjustment ([Velox type](https://github.com/facebookincubator/velox/blob/68c4501e9ae33a2d6041cc3f9ce40e0bec376fc7/velox/type/Type.h#L1172-L1201)). The shared min/max factory dispatches `TypeKind::TIMESTAMP` to the existing `SimpleNumericMin/MaxAggregate<Timestamp>` implementation and carries the supplied result type through ([factory](https://github.com/facebookincubator/velox/blob/68c4501e9ae33a2d6041cc3f9ce40e0bec376fc7/velox/functions/lib/aggregates/MinMaxAggregateBase.cpp#L595-L630)). Spark's registration configures that implementation with microsecond precision ([registration](https://github.com/facebookincubator/velox/blob/68c4501e9ae33a2d6041cc3f9ce40e0bec376fc7/velox/functions/sparksql/aggregates/MinMaxAggregate. cpp#L23-L78)). The Gluten changes preserve `TIMESTAMP_NTZ` as `TIMESTAMP_UTC` through the function signature and Substrait output type, then allow the plan to reach native validation. This does not assume every aggregate function supports `TIMESTAMP_NTZ`: each aggregate is still checked by native signature validation, and unsupported functions continue to fall back. The behavior specifically enabled and asserted here is the `min`/`max` path used by Delta statistics. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
