felipepessoto commented on code in PR #12967:
URL: https://github.com/apache/gluten/pull/12967#discussion_r4077434434
##########
cpp/velox/substrait/VeloxToSubstraitType.cc:
##########
@@ -31,6 +31,14 @@ const ::substrait::Type&
VeloxToSubstraitTypeConvertor::toSubstraitType(
substraitType->set_allocated_date(substraitDate);
return *substraitType;
}
+ if (type->equivalent(*velox::TIMESTAMP_UTC())) {
+ auto substraitPrecisionTimestamp =
+
google::protobuf::Arena::CreateMessage<::substrait::Type_PrecisionTimestamp>(&arena);
+ substraitPrecisionTimestamp->set_precision(6);
+
substraitPrecisionTimestamp->set_nullability(::substrait::Type_Nullability_NULLABILITY_NULLABLE);
+
substraitType->set_allocated_precision_timestamp(substraitPrecisionTimestamp);
+ return *substraitType;
+ }
Review Comment:
This converter currently accepts only (arena, type) , with no nullability
argument. The parser does not retain nullability in the Velox type, and
existing conversion branches—including ordinary TIMESTAMP —also emit NULLABLE
. Local probes confirmed that required inputs become nullable for both
timestamp types, so this behavior is not introduced by the NTZ branch. @rui-mo,
should we open a separate issue to track preserving schema nullability end to
end, since this is existing converter-wide behavior?
--
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]
