tanclary commented on code in PR #3569:
URL: https://github.com/apache/calcite/pull/3569#discussion_r1435825219
##########
cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java:
##########
@@ -192,6 +193,9 @@ private String translateMatch(RexNode condition) {
private static Object literalValue(RexLiteral literal) {
Comparable<?> value = RexLiteral.value(literal);
switch (literal.getTypeName()) {
+ case TIMESTAMP_TZ:
Review Comment:
I think we should follow existing naming convention, if local time zone is
spelled out then maybe this should be `TIMESTAMP_WITH_TIME_ZONE`
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystemImpl.java:
##########
@@ -126,8 +128,10 @@ public abstract class RelDataTypeSystemImpl implements
RelDataTypeSystem {
return 65536;
case TIME:
case TIME_WITH_LOCAL_TIME_ZONE:
+ case TIME_TZ:
Review Comment:
same naming comment as above for `TIME`
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeName.java:
##########
@@ -65,12 +65,16 @@ public enum SqlTypeName {
DATE(PrecScale.NO_NO, false, Types.DATE, SqlTypeFamily.DATE),
TIME(PrecScale.NO_NO | PrecScale.YES_NO, false, Types.TIME,
SqlTypeFamily.TIME),
- TIME_WITH_LOCAL_TIME_ZONE(PrecScale.NO_NO | PrecScale.YES_NO, false,
Types.OTHER,
+ TIME_WITH_LOCAL_TIME_ZONE(PrecScale.NO_NO | PrecScale.YES_NO, false,
Types.TIME,
+ SqlTypeFamily.TIME),
+ TIME_TZ(PrecScale.NO_NO | PrecScale.YES_NO, false, Types.TIME,
Review Comment:
Going through this PR I'm also not opposed to changing the local time zones
to something like `TIMESTAMP_LTZ`, I think we actually use that in some parts
of the code. Either way should probably be consistent. What do you think?
##########
core/src/main/java/org/apache/calcite/sql/SqlBasicTypeNameSpec.java:
##########
@@ -245,16 +249,31 @@ private static boolean isWithLocalTimeZoneDef(SqlTypeName
typeName) {
}
}
+ private static boolean isWithTimeZoneDef(SqlTypeName typeName) {
Review Comment:
Alternatively you could add a set/list to `SqlTypeName` (see
`DATETIME_TYPES` for example) and then just use `.contains()` or something
similar.
##########
core/src/main/java/org/apache/calcite/sql/parser/SqlParserUtil.java:
##########
@@ -351,6 +356,31 @@ public static SqlTimeLiteral parseTimeLiteral(String s,
SqlParserPos pos) {
return SqlLiteral.createTime(t, pt.getPrecision(), pos);
}
+ public static SqlTimeTzLiteral parseTimeTzLiteral(
+ String s, SqlParserPos pos) {
+ // We expect the string to end in a timezone.
+ int lastSpace = s.lastIndexOf(" ");
Review Comment:
Should this be marked `final`?
##########
core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java:
##########
@@ -473,6 +473,9 @@ public static SqlOperandTypeChecker variadic(
public static final SqlSingleOperandTypeChecker TIMESTAMP_LTZ =
new TypeNameChecker(SqlTypeName.TIMESTAMP_WITH_LOCAL_TIME_ZONE);
+ public static final SqlSingleOperandTypeChecker TIMESTAMP_TZ =
Review Comment:
Needs comment
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeAssignmentRule.java:
##########
@@ -175,13 +175,21 @@ private SqlTypeAssignmentRule(
rules.add(SqlTypeName.TIME_WITH_LOCAL_TIME_ZONE,
EnumSet.of(SqlTypeName.TIME_WITH_LOCAL_TIME_ZONE));
+ // TIME WITH TIME ZONE is assignable from...
+ rules.add(SqlTypeName.TIME_TZ,
Review Comment:
This can probably be one line right
##########
cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraFilter.java:
##########
@@ -192,6 +193,9 @@ private String translateMatch(RexNode condition) {
private static Object literalValue(RexLiteral literal) {
Comparable<?> value = RexLiteral.value(literal);
switch (literal.getTypeName()) {
+ case TIMESTAMP_TZ:
Review Comment:
If you make this change you can move this `case` below the one beneath to
preserve alphabetical order.
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeName.java:
##########
@@ -309,7 +314,10 @@ public enum SqlTypeName {
* matches the given name, or throws {@link IllegalArgumentException}; never
* returns null. */
public static SqlTypeName lookup(String tag) {
- String tag2 = tag.replace(' ', '_');
+ // Special handling for TIME WITH TIME ZONE and
+ // TIMESTAMP WITH TIME ZONE, whose names are TIME_TZ and TIMESTAMP_TZ
+ final String tag1 = tag.replace("WITH TIME ZONE", "TZ");
Review Comment:
This is kind of personal preference but you could simplify this with a
single regex like
` return re.sub(r"(?i)WITH TIME ZONE|\s", "TZ", tag)`
--
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]
