gortiz commented on code in PR #14398:
URL: https://github.com/apache/pinot/pull/14398#discussion_r1832508244
##########
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
--
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]
