chunxiaozheng commented on code in PR #14398:
URL: https://github.com/apache/pinot/pull/14398#discussion_r1834321250
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java:
##########
@@ -273,11 +278,29 @@ public RelDataType toType(RelDataTypeFactory typeFactory)
{
}
},
TIMESTAMP(LONG, NullValuePlaceHolder.LONG) {
+ @Override
+ public RelDataType toType(RelDataTypeFactory typeFactory) {
+ return
typeFactory.createSqlType(SqlTypeName.TIMESTAMP_WITH_LOCAL_TIME_ZONE);
+ }
+ },
+ TIMESTAMP_NTZ(LONG, NullValuePlaceHolder.LONG) {
@Override
public RelDataType toType(RelDataTypeFactory typeFactory) {
return typeFactory.createSqlType(SqlTypeName.TIMESTAMP);
}
},
Review Comment:
> I guess you did this to follow the Spark terminology, but apart from my
preference to following Postgres terminology I think this is breaking backward
compatibility. Before this PR a Pinot TIMESTAMP was converted to Calcite
TIMESTAMP and now we are converting it into a TIMESTAMP WITH LOCAL TIME ZONE.
>
> I may be wrong but I think what we should be doing here is to return that
our TIMESTAMP_TZ returns a Calcite TIMESTAMP_TZ
Thanks for your review! From my understanding, the TIMESTAMP type in Calcite
has no time zone, but the TIMESTAMP type in pinot is all using
java.sql.Timestamp to explain, which is with the local time zone, so i made
changes here.
In order to minimize modifications to the TIMESTAMP type in pinot, i
referred to the data type in Spark and extended the TIMESTAMP_NTZ type.
--
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]
