[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer by docete · Pull Request #1687 · apache/calcite

docete · 2019-12-25T04:31:34Z

Introduce TIME/TIMESTAMP WITH TIME ZONE types
Introduce TimeWithTimeZone as internal representation of TIME WITH TIME ZONE type
Introduce TimestampWithTimeZone as internal representation of TIMESTAMP WITH TIME ZONE type
Introduce coercion rules for TIME/TIMESTAMP WITH TIME ZONE types

amaliujia

Thanks for you PR!

amaliujia · 2019-12-25T04:58:02Z

core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java

-import org.apache.calcite.util.ControlFlowException;
-import org.apache.calcite.util.Pair;
-import org.apache.calcite.util.Util;
+import org.apache.calcite.util.*;


Don't use *, import classes one by one.

amaliujia · 2019-12-25T05:02:54Z

core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java

    return scaleIntervalToNumber(sourceType, targetType, convert);
  }

+  private Expression converterToDate(RelDataType sourceType, Expression operand) {


I would prefer to split this refactoring with adding the new types into two PRs, as it will make clear what is added by this PR and what is newly added code. By doing so we could better review the new code.

Let's see what are other reviewers' opinions.

The aim of the refactoring is make sure the length of a method is smaller than 370, and avoid complaining by checkstyle. I will split the refactoring part to a separate commit, that would clarify the PR.

amaliujia · 2019-12-25T05:22:23Z

core/src/main/java/org/apache/calcite/rex/RexBuilder.java

+    case TIME_WITH_TIME_ZONE:
+      return new TimeWithTimeZoneString(0, 0, 0, "UTC");
+    case TIMESTAMP_WITH_TIME_ZONE:
+      return new TimestampWithTimeZoneString(0, 0, 0, 0, 0, 0, "UTC");


Shouldn't 0 is January 1, 1970 UTC, assuming the millis are epoch millis (count from 1970-01-01 UTC)

After double checked the ZERO of TIMESTAMP type, I think your are right. ZERO should be January 1, 1970 UTC.

amaliujia · 2019-12-25T05:27:18Z

core/src/main/java/org/apache/calcite/util/TimeWithTimeZone.java

+      return false;
+    }
+    TimeWithTimeZone that = (TimeWithTimeZone) obj;
+    return milliOfDay == that.milliOfDay && timeZone == that.timeZone;


Should call timeZone.equals(that.timeZone) otherwise it is comparing the reference but not real value?

Yes, you're right.

amaliujia · 2019-12-25T05:29:07Z

core/src/main/java/org/apache/calcite/util/TimestampWithTimeZone.java

+public class TimestampWithTimeZone implements Comparable<TimestampWithTimeZone>, Serializable {
+  //~ Instance fields --------------------------------------------------------
+
+  private final long millisecond;


is this epoch millis or just millis? If it is epoch millis, can you update comments to reflect it?

The millisecond should be a zone-less TIMESTAMP(the internal representation of TIMESTAMP WITHOUT TIME ZONE). The java doc of the constructor has made it clearly.

Can you point to me the class that represents TIMESTAMP WITHOUT TIME ZONE in Calcite? I want to double check what type of millis that saves internally?

Just a long value. You can see BuiltInMethod.STRING_TO_TIMESTAMP

amaliujia · 2019-12-25T05:29:28Z

core/src/main/java/org/apache/calcite/util/TimestampWithTimeZone.java

+      return false;
+    }
+    TimestampWithTimeZone that = (TimestampWithTimeZone) obj;
+    return this.millisecond == that.millisecond && this.timeZone == that.timeZone;


this.timeZone.equals(that.timeZone)

amaliujia · 2019-12-25T05:30:08Z

core/src/test/java/org/apache/calcite/test/RexProgramTest.java

-import org.apache.calcite.util.TimestampString;
-import org.apache.calcite.util.TimestampWithTimeZoneString;
-import org.apache.calcite.util.Util;
+import org.apache.calcite.util.*;


Don't use *. Import classes one by one.

amaliujia · 2019-12-25T05:31:18Z

core/src/test/java/org/apache/calcite/test/RexProgramTest.java

+    final RexLiteral literalTimestampWithTZ =
+        rexBuilder.makeTimestampWithTimeZoneLiteral(
+            new TimestampWithTimeZoneString(
+                new TimestampString(2017, 7, 20, 8, 23, 45),


Can you test up to millis?

I noticed the TIMESTAMP and TIMESTAMP WITH LOCAL TIME ZONE also ignored precision in RexToLixTranslator. See cast(TIMESTAMP as CHAR), I think we should follow that. And we could open another ticket to support precision of them.

amaliujia · 2019-12-25T06:01:36Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

        .getMillisSinceEpoch();
  }

+  public static int timeWithTimeZoneToTime(TimeWithTimeZone ttz) {


Do we need javadoc for these public static methods?

I think the method name has documented it?

docete · 2019-12-25T09:01:19Z

rebased to split the refactoring to a separate commit.

amaliujia · 2019-12-26T23:49:45Z

core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java

+    // SQL 2011 Part2 Section 4.6 General Rules 13.d
+    case TIMESTAMP_WITH_TIME_ZONE:
+      convert = Expressions.convert_(
+        Expressions.call(BuiltInMethod.FLOOR_DIV.method,


implement a timestampWithoutTimeZoneToDate and call it here?

Not needed!

Section 6.13 General Rules 13.d shows this can be computed as "CAST ( CAST ( VE AS TIMESTAMP WITHOUT TIME ZONE ) AS DATE )"

the underlying TimestampWithTimeZone holds the same long value with Timestamp type, current implementation is more simple

Yes. By reading the SQL standard I got the idea of the conversion path.

This suggestion is more from readability perspective. If you implements one more function that then code readers will know the equivalent implementation is CAST ( CAST ( VE AS TIMESTAMP WITHOUT TIME ZONE ) AS DATE ).

I had to read SQL standard cause I wasn't sure why there is a "magic" operation after casting to TIMESTAMP WITHOUT TIME ZONE.

Or maybe you can add a comment instead to say the implementation here is CAST ( CAST ( VE AS TIMESTAMP WITHOUT TIME ZONE ) AS DATE )

amaliujia · 2019-12-26T23:51:10Z

core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java

          Expressions.call(BuiltInMethod.TIME_ZONE.method, root)));
+      break;
+    // SQL 2011 Part2 Section 4.6 General Rules 15.e
+    case TIMESTAMP_WITH_TIME_ZONE:


implement a timestampWithoutTimeZoneToTimeWithoutTimeZone and call it here?

amaliujia · 2019-12-26T23:53:33Z

core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java

          operand,
          Expressions.call(BuiltInMethod.TIME_ZONE.method, root)));
+      break;
+    // SQL 2011 Part2 Section 4.6 General Rules 15.e


I didn't find "general rules" under section 4.6 (was looking at page no.36)

SQL 2011 Part2 Section 4.6 Table 3 gives a very clear conversion table.

Sorry, it should be Section 6.13 (begin with p243)

amaliujia · 2019-12-26T23:55:49Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

+    return localTime + " " + ttz.getTimeZone().getID();
+  }
+
+  public static TimeWithTimeZone toTimeWithTimeZone(String v, TimeZone sesstionTimeZone) {


rename to stringToTimeWithTimeZone?

amaliujia · 2019-12-26T23:56:46Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

+    return new TimeWithTimeZone(milliOfDay, tstz.getTimeZone());
+  }
+
+  public static TimestampWithTimeZone toTimestampWithTimeZone(String v, TimeZone sessionTimeZone) {


Maybe also rename and include string?

amaliujia · 2019-12-26T23:58:40Z

core/src/main/java/org/apache/calcite/util/TimestampWithTimeZone.java

+public class TimestampWithTimeZone implements Comparable<TimestampWithTimeZone>, Serializable {
+  //~ Instance fields --------------------------------------------------------
+
+  private final long millisecond;


Can you point to me the class that represents TIMESTAMP WITHOUT TIME ZONE in Calcite? I want to double check what type of millis that saves internally?

* Introduce TIME/TIMESTAMP WITH TIME ZONE types * Introduce TimeWithTimeZone as internal representation of TIME WITH TIME ZONE type * Introduce TimestampWithTimeZone as internal representation of TIMESTAMP WITH TIME ZONE type * Introduce coercion rules for TIME/TIMESTAMP WITH TIME ZONE types

docete · 2020-01-07T09:34:14Z

rebase to resolve all conflicts

docete · 2020-01-07T09:41:43Z

@julianhyde Could you take a look at this if you are free? Hopefully I touched all the right places and didn't missing any thing.

amaliujia · 2020-09-07T18:01:37Z

@docete

Would you like to pick up this work again? I can help on review/merge your work.

docete · 2020-09-24T02:34:04Z

@amaliujia Sorry for the late reply, I will rebase this PR in recent days.

ian-bertolacci · 2022-02-25T18:23:18Z

What's the status of this work? Is there someone that can get this to completion? This is a pretty significant hole in Calcite's SQL support.

amaliujia reviewed Dec 25, 2019

View reviewed changes

docete force-pushed the CALCITE-3612 branch from 56f5185 to 28879ae Compare December 25, 2019 09:00

vlsi force-pushed the master branch from 484b112 to 1afe1e4 Compare December 26, 2019 08:13

amaliujia reviewed Dec 26, 2019

View reviewed changes

vlsi force-pushed the master branch from 49cb002 to 8768a23 Compare December 29, 2019 12:07

docete force-pushed the CALCITE-3612 branch from 33e31bc to cdfd748 Compare January 7, 2020 09:26

docete added 4 commits January 7, 2020 17:27

[CALCITE-3612] Refactor long method to avoid complain of checkstyle

c47efaf

fixup: address Rui's comments

6989b23

fixup: update comments for clearness

fe00af0

docete force-pushed the CALCITE-3612 branch from cdfd748 to fe00af0 Compare January 7, 2020 09:32

hsyuan force-pushed the master branch from e42a79b to a0ef3c9 Compare March 29, 2020 16:52

vlsi force-pushed the master branch from e4458fa to 5462be9 Compare July 17, 2020 20:19

vlsi force-pushed the master branch from fd20efd to 3311d45 Compare December 9, 2020 20:10

julianhyde force-pushed the master branch from 52c1284 to d4e1eea Compare March 1, 2021 02:56

vlsi force-pushed the master branch from 7f65cf2 to 4bc9166 Compare March 24, 2021 09:43

zabetak force-pushed the master branch from f14cf4c to dcbc493 Compare March 10, 2022 09:13

julianhyde force-pushed the main branch from fa65a2e to 1226d1a Compare June 20, 2022 20:27

asfgit force-pushed the main branch from 9fc50f2 to e2f949d Compare September 10, 2022 16:37

asfgit force-pushed the main branch from f8f8a51 to a326bd2 Compare January 25, 2023 07:39

julianhyde force-pushed the main branch 2 times, most recently from 8a5cf83 to cf7f71b Compare June 8, 2023 21:21

tanclary force-pushed the main branch from 4804912 to 00db001 Compare September 6, 2023 00:42

libenchao force-pushed the main branch from 47db81a to 0be8eae Compare November 10, 2023 13:18

F21 force-pushed the main branch from 7d38212 to cacf36a Compare February 17, 2025 03:33

asolimando force-pushed the main branch from 19400ab to 2d5ec10 Compare May 28, 2025 13:26

Conversation

docete commented Dec 25, 2019

Uh oh!

amaliujia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amaliujia Dec 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amaliujia Dec 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

docete Dec 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

docete commented Dec 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

docete Dec 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

docete commented Jan 7, 2020

Uh oh!

docete commented Jan 7, 2020

Uh oh!

amaliujia Dec 25, 2019 •

edited

Loading

amaliujia Dec 25, 2019 •

edited

Loading

docete Dec 25, 2019 •

edited

Loading

docete Dec 27, 2019 •

edited

Loading