You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2019/12/25 04:31:35 UTC
[GitHub] [calcite] docete opened a new pull request #1687: [CALCITE-3612]
Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
docete opened a new pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687
* 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
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361268673
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java
##########
@@ -48,10 +48,7 @@
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
import org.apache.calcite.sql.type.SqlTypeUtil;
import org.apache.calcite.sql.validate.SqlConformance;
-import org.apache.calcite.util.BuiltInMethod;
-import org.apache.calcite.util.ControlFlowException;
-import org.apache.calcite.util.Pair;
-import org.apache.calcite.util.Util;
+import org.apache.calcite.util.*;
Review comment:
Sure
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361258387
##########
File path: core/src/test/java/org/apache/calcite/test/RexProgramTest.java
##########
@@ -42,14 +42,7 @@
import org.apache.calcite.sql.type.ReturnTypes;
import org.apache.calcite.sql.type.SqlTypeAssignmentRule;
import org.apache.calcite.sql.type.SqlTypeName;
-import org.apache.calcite.util.DateString;
-import org.apache.calcite.util.ImmutableBitSet;
-import org.apache.calcite.util.NlsString;
-import org.apache.calcite.util.TestUtil;
-import org.apache.calcite.util.TimeString;
-import org.apache.calcite.util.TimestampString;
-import org.apache.calcite.util.TimestampWithTimeZoneString;
-import org.apache.calcite.util.Util;
+import org.apache.calcite.util.*;
Review comment:
Don't use *. Import classes one by one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361741594
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java
##########
@@ -341,6 +344,16 @@ private Expression converterToDate(RelDataType sourceType, Expression operand) {
BuiltInMethod.TIMESTAMP_WITH_LOCAL_TIME_ZONE_TO_DATE.method,
operand,
Expressions.call(BuiltInMethod.TIME_ZONE.method, root)));
+ break;
+ // SQL 2011 Part2 Section 4.6 General Rules 13.d
+ case TIMESTAMP_WITH_TIME_ZONE:
+ convert = Expressions.convert_(
+ Expressions.call(BuiltInMethod.FLOOR_DIV.method,
Review comment:
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 )`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361270242
##########
File path: core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
##########
@@ -2021,6 +2023,137 @@ public static Long toTimestampWithLocalTimeZone(String v, TimeZone timeZone) {
.getMillisSinceEpoch();
}
+ public static int timeWithTimeZoneToTime(TimeWithTimeZone ttz) {
Review comment:
I think the method name has documented it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361269219
##########
File path: core/src/main/java/org/apache/calcite/util/TimeWithTimeZone.java
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import java.io.Serializable;
+import java.util.Objects;
+import java.util.TimeZone;
+
+/**
+ * A Time with time zone. It is immutable.
+ */
+public class TimeWithTimeZone implements Comparable<TimeWithTimeZone>, Serializable {
+ //~ Instance fields --------------------------------------------------------
+
+ private final int milliOfDay;
+ private final TimeZone timeZone;
+
+ /**
+ * Constructor. Obtains an instance of {@link TimeWithTimeZone} from a millisecond of day
+ * and a {@link TimeZone}.
+ *
+ * @param milliOfDay the number of milliseconds of the day.
+ * @param timeZone the time-zone to use.
+ */
+ public TimeWithTimeZone(int milliOfDay, TimeZone timeZone) {
+ this.milliOfDay = milliOfDay;
+ this.timeZone = timeZone;
+ }
+
+ public TimeWithTimeZone(int milliOfDay, String tz) {
+ this.milliOfDay = milliOfDay;
+ this.timeZone = TimeZone.getTimeZone(tz);
+ }
+
+ /**
+ * Converts this time to epoch millis based on 1970-01-01Z.
+ *
+ * @return the epoch milli value
+ */
+ private int toEpochMilli() {
+ int nod = milliOfDay;
+ int offsetMillis = timeZone.getOffset(nod);
+ return nod - offsetMillis;
+ }
+
+ /**
+ * Compare the {@link TimeWithTimeZone} to another time.
+ * <p>
+ * The comparison is based first on the UTC equivalent instant, then on the local time.
+ * See {@link java.time.OffsetTime}
+ *
+ * @param o the other time to compare to.
+ * @return the comparator value, negative if less, positive if greater.
+ */
+ @Override public int compareTo(TimeWithTimeZone o) {
+ if (timeZone.equals(o.timeZone)) {
+ return Integer.compare(milliOfDay, o.milliOfDay);
+ }
+ int compare = Integer.compare(toEpochMilli(), o.toEpochMilli());
+ if (compare == 0) {
+ compare = Integer.compare(milliOfDay, o.milliOfDay);
+ }
+ return compare;
+ }
+
+ @Override public int hashCode() {
+ return Objects.hash(milliOfDay, timeZone);
+ }
+
+ @Override public boolean equals(Object obj) {
+ if (!(obj instanceof TimeWithTimeZone)) {
+ return false;
+ }
+ TimeWithTimeZone that = (TimeWithTimeZone) obj;
+ return milliOfDay == that.milliOfDay && timeZone == that.timeZone;
Review comment:
Yes, you're right.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361255554
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java
##########
@@ -48,10 +48,7 @@
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
import org.apache.calcite.sql.type.SqlTypeUtil;
import org.apache.calcite.sql.validate.SqlConformance;
-import org.apache.calcite.util.BuiltInMethod;
-import org.apache.calcite.util.ControlFlowException;
-import org.apache.calcite.util.Pair;
-import org.apache.calcite.util.Util;
+import org.apache.calcite.util.*;
Review comment:
Don't use `*`, import classes one by one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on issue #1687: [CALCITE-3612] Add
TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on issue #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#issuecomment-571511388
@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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361258290
##########
File path: core/src/main/java/org/apache/calcite/util/TimestampWithTimeZone.java
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import java.io.Serializable;
+import java.util.Objects;
+import java.util.TimeZone;
+
+/**
+ * A Timestamp with time zone. It is immutable.
+ */
+public class TimestampWithTimeZone implements Comparable<TimestampWithTimeZone>, Serializable {
+ //~ Instance fields --------------------------------------------------------
+
+ private final long millisecond;
Review comment:
is this epoch millis or just millis? If it is epoch millis, can you update comments to reflect it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361258486
##########
File path: core/src/test/java/org/apache/calcite/test/RexProgramTest.java
##########
@@ -2223,15 +2212,27 @@ private void checkExponentialCnf(int n) {
final RexLiteral literalTimeLTZ =
rexBuilder.makeTimeWithLocalTimeZoneLiteral(
new TimeString(1, 23, 45), 0);
+ final RexLiteral timeChar = rexBuilder.makeLiteral("12:34:45");
final RexLiteral timeLTZChar1 = rexBuilder.makeLiteral("12:34:45 America/Los_Angeles");
final RexLiteral timeLTZChar2 = rexBuilder.makeLiteral("12:34:45 UTC");
final RexLiteral timeLTZChar3 = rexBuilder.makeLiteral("12:34:45 GMT+01");
+ final RexLiteral timestampChar = rexBuilder.makeLiteral("2011-07-20 12:34:56");
final RexLiteral timestampLTZChar1 = rexBuilder.makeLiteral("2011-07-20 12:34:56 Asia/Tokyo");
final RexLiteral timestampLTZChar2 = rexBuilder.makeLiteral("2011-07-20 12:34:56 GMT+01");
final RexLiteral timestampLTZChar3 = rexBuilder.makeLiteral("2011-07-20 12:34:56 UTC");
final RexLiteral literalTimestampLTZ =
rexBuilder.makeTimestampWithLocalTimeZoneLiteral(
new TimestampString(2011, 7, 20, 8, 23, 45), 0);
+ final RexLiteral literalTimeWithTZ =
+ rexBuilder.makeTimeWithTimeZoneLiteral(
+ new TimeWithTimeZoneString(
+ new TimeString(1, 23, 45),
+ TimeZone.getTimeZone("Asia/Shanghai")), 0);
+ final RexLiteral literalTimestampWithTZ =
+ rexBuilder.makeTimestampWithTimeZoneLiteral(
+ new TimestampWithTimeZoneString(
+ new TimestampString(2017, 7, 20, 8, 23, 45),
Review comment:
Can you test up to millis?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361270030
##########
File path: core/src/main/java/org/apache/calcite/util/TimestampWithTimeZone.java
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import java.io.Serializable;
+import java.util.Objects;
+import java.util.TimeZone;
+
+/**
+ * A Timestamp with time zone. It is immutable.
+ */
+public class TimestampWithTimeZone implements Comparable<TimestampWithTimeZone>, Serializable {
+ //~ Instance fields --------------------------------------------------------
+
+ private final long millisecond;
Review comment:
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361579628
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java
##########
@@ -341,6 +344,16 @@ private Expression converterToDate(RelDataType sourceType, Expression operand) {
BuiltInMethod.TIMESTAMP_WITH_LOCAL_TIME_ZONE_TO_DATE.method,
operand,
Expressions.call(BuiltInMethod.TIME_ZONE.method, root)));
+ break;
+ // SQL 2011 Part2 Section 4.6 General Rules 13.d
+ case TIMESTAMP_WITH_TIME_ZONE:
+ convert = Expressions.convert_(
+ Expressions.call(BuiltInMethod.FLOOR_DIV.method,
Review comment:
Not needed!
1) Section 6.13 General Rules 13.d shows this can be computed as "CAST ( CAST ( VE AS TIMESTAMP WITHOUT TIME ZONE ) AS DATE )"
2) My underlying TimestampWithTimeZone holds the same long value with Timestamp type, current implementation is more simple
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361257782
##########
File path: core/src/main/java/org/apache/calcite/rex/RexBuilder.java
##########
@@ -1345,6 +1384,10 @@ private static Comparable zeroValue(RelDataType type) {
return new TimeString(0, 0, 0);
case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
return new TimestampString(0, 0, 0, 0, 0, 0);
+ 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");
Review comment:
Shouldn't 0 is `January 1, 1970 UTC`, assuming the millis are epoch millis (count from 1970-01-01 UTC)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361261358
##########
File path: core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
##########
@@ -2021,6 +2023,137 @@ public static Long toTimestampWithLocalTimeZone(String v, TimeZone timeZone) {
.getMillisSinceEpoch();
}
+ public static int timeWithTimeZoneToTime(TimeWithTimeZone ttz) {
Review comment:
Do we need javadoc for these `public static` methods?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on issue #1687: [CALCITE-3612] Add
TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on issue #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#issuecomment-571508858
rebase to resolve all conflicts
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361551655
##########
File path: core/src/main/java/org/apache/calcite/util/TimestampWithTimeZone.java
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import java.io.Serializable;
+import java.util.Objects;
+import java.util.TimeZone;
+
+/**
+ * A Timestamp with time zone. It is immutable.
+ */
+public class TimestampWithTimeZone implements Comparable<TimestampWithTimeZone>, Serializable {
+ //~ Instance fields --------------------------------------------------------
+
+ private final long millisecond;
Review comment:
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?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361258163
##########
File path: core/src/main/java/org/apache/calcite/util/TimeWithTimeZone.java
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import java.io.Serializable;
+import java.util.Objects;
+import java.util.TimeZone;
+
+/**
+ * A Time with time zone. It is immutable.
+ */
+public class TimeWithTimeZone implements Comparable<TimeWithTimeZone>, Serializable {
+ //~ Instance fields --------------------------------------------------------
+
+ private final int milliOfDay;
+ private final TimeZone timeZone;
+
+ /**
+ * Constructor. Obtains an instance of {@link TimeWithTimeZone} from a millisecond of day
+ * and a {@link TimeZone}.
+ *
+ * @param milliOfDay the number of milliseconds of the day.
+ * @param timeZone the time-zone to use.
+ */
+ public TimeWithTimeZone(int milliOfDay, TimeZone timeZone) {
+ this.milliOfDay = milliOfDay;
+ this.timeZone = timeZone;
+ }
+
+ public TimeWithTimeZone(int milliOfDay, String tz) {
+ this.milliOfDay = milliOfDay;
+ this.timeZone = TimeZone.getTimeZone(tz);
+ }
+
+ /**
+ * Converts this time to epoch millis based on 1970-01-01Z.
+ *
+ * @return the epoch milli value
+ */
+ private int toEpochMilli() {
+ int nod = milliOfDay;
+ int offsetMillis = timeZone.getOffset(nod);
+ return nod - offsetMillis;
+ }
+
+ /**
+ * Compare the {@link TimeWithTimeZone} to another time.
+ * <p>
+ * The comparison is based first on the UTC equivalent instant, then on the local time.
+ * See {@link java.time.OffsetTime}
+ *
+ * @param o the other time to compare to.
+ * @return the comparator value, negative if less, positive if greater.
+ */
+ @Override public int compareTo(TimeWithTimeZone o) {
+ if (timeZone.equals(o.timeZone)) {
+ return Integer.compare(milliOfDay, o.milliOfDay);
+ }
+ int compare = Integer.compare(toEpochMilli(), o.toEpochMilli());
+ if (compare == 0) {
+ compare = Integer.compare(milliOfDay, o.milliOfDay);
+ }
+ return compare;
+ }
+
+ @Override public int hashCode() {
+ return Objects.hash(milliOfDay, timeZone);
+ }
+
+ @Override public boolean equals(Object obj) {
+ if (!(obj instanceof TimeWithTimeZone)) {
+ return false;
+ }
+ TimeWithTimeZone that = (TimeWithTimeZone) obj;
+ return milliOfDay == that.milliOfDay && timeZone == that.timeZone;
Review comment:
Should call `timeZone.equals(that.timeZone`) otherwise it is comparing the reference but not real value?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361270113
##########
File path: core/src/test/java/org/apache/calcite/test/RexProgramTest.java
##########
@@ -2223,15 +2212,27 @@ private void checkExponentialCnf(int n) {
final RexLiteral literalTimeLTZ =
rexBuilder.makeTimeWithLocalTimeZoneLiteral(
new TimeString(1, 23, 45), 0);
+ final RexLiteral timeChar = rexBuilder.makeLiteral("12:34:45");
final RexLiteral timeLTZChar1 = rexBuilder.makeLiteral("12:34:45 America/Los_Angeles");
final RexLiteral timeLTZChar2 = rexBuilder.makeLiteral("12:34:45 UTC");
final RexLiteral timeLTZChar3 = rexBuilder.makeLiteral("12:34:45 GMT+01");
+ final RexLiteral timestampChar = rexBuilder.makeLiteral("2011-07-20 12:34:56");
final RexLiteral timestampLTZChar1 = rexBuilder.makeLiteral("2011-07-20 12:34:56 Asia/Tokyo");
final RexLiteral timestampLTZChar2 = rexBuilder.makeLiteral("2011-07-20 12:34:56 GMT+01");
final RexLiteral timestampLTZChar3 = rexBuilder.makeLiteral("2011-07-20 12:34:56 UTC");
final RexLiteral literalTimestampLTZ =
rexBuilder.makeTimestampWithLocalTimeZoneLiteral(
new TimestampString(2011, 7, 20, 8, 23, 45), 0);
+ final RexLiteral literalTimeWithTZ =
+ rexBuilder.makeTimeWithTimeZoneLiteral(
+ new TimeWithTimeZoneString(
+ new TimeString(1, 23, 45),
+ TimeZone.getTimeZone("Asia/Shanghai")), 0);
+ final RexLiteral literalTimestampWithTZ =
+ rexBuilder.makeTimestampWithTimeZoneLiteral(
+ new TimestampWithTimeZoneString(
+ new TimestampString(2017, 7, 20, 8, 23, 45),
Review comment:
OK
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361268928
##########
File path: core/src/main/java/org/apache/calcite/rex/RexBuilder.java
##########
@@ -1345,6 +1384,10 @@ private static Comparable zeroValue(RelDataType type) {
return new TimeString(0, 0, 0);
case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
return new TimestampString(0, 0, 0, 0, 0, 0);
+ 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");
Review comment:
After double checked the ZERO of TIMESTAMP type, I think your are right. ZERO should be `January 1, 1970 UTC`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361579647
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java
##########
@@ -376,6 +394,17 @@ private Expression converterToTime(RelDataType sourceType, Expression operand) {
BuiltInMethod.TIMESTAMP_WITH_LOCAL_TIME_ZONE_TO_TIME.method,
operand,
Expressions.call(BuiltInMethod.TIME_ZONE.method, root)));
+ break;
+ // SQL 2011 Part2 Section 4.6 General Rules 15.e
+ case TIMESTAMP_WITH_TIME_ZONE:
Review comment:
ditto
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361551515
##########
File path: core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
##########
@@ -2021,6 +2023,137 @@ public static Long toTimestampWithLocalTimeZone(String v, TimeZone timeZone) {
.getMillisSinceEpoch();
}
+ public static int timeWithTimeZoneToTime(TimeWithTimeZone ttz) {
+ return ttz.getMilliOfDay();
+ }
+
+ public static TimestampWithTimeZone timeWithTimeZoneToTimestampWithTimeZone(int date,
+ TimeWithTimeZone ttz) {
+ long millisecond = date * DateTimeUtils.MILLIS_PER_DAY + ttz.getMilliOfDay();
+ return new TimestampWithTimeZone(millisecond, ttz.getTimeZone());
+ }
+
+ public static String timeWithTimeZoneToString(TimeWithTimeZone ttz) {
+ String localTime = DateTimeUtils.unixTimeToString(ttz.getMilliOfDay());
+ return localTime + " " + ttz.getTimeZone().getID();
+ }
+
+ public static TimeWithTimeZone toTimeWithTimeZone(String v, TimeZone sesstionTimeZone) {
+ if (v == null) {
+ return null;
+ }
+
+ final int colon1 = v.indexOf(':', 0);
+ int hour;
+ int minute;
+ int second;
+ int milli;
+ TimeZone tz = sesstionTimeZone;
+ if (colon1 < 0) {
+ hour = Integer.parseInt(v.trim());
+ minute = 1;
+ second = 1;
+ milli = 0;
+ } else {
+ hour = Integer.parseInt(v.substring(0, colon1).trim());
+ final int colon2 = v.indexOf(':', colon1 + 1);
+ if (colon2 < 0) {
+ minute = Integer.parseInt(v.substring(colon1 + 1).trim());
+ second = 1;
+ milli = 0;
+ } else {
+ minute = Integer.parseInt(v.substring(colon1 + 1, colon2).trim());
+ int dot = v.indexOf('.', colon2);
+ int space = v.indexOf(' ', colon2);
+ if (dot < 0) {
+ if (space < 0) {
+ second = Integer.parseInt(v.substring(colon2 + 1).trim());
+ milli = 0;
+ } else {
+ second = Integer.parseInt(v.substring(colon2 + 1, space).trim());
+ milli = 0;
+ tz = TimeZone.getTimeZone(v.substring(space + 1).trim());
+ }
+ } else {
+ if (space < 0) {
+ second = Integer.parseInt(v.substring(colon2 + 1, dot).trim());
+ milli = parseFraction(v.substring(dot + 1).trim(), 100);
+ } else {
+ second = Integer.parseInt(v.substring(colon2 + 1, dot).trim());
+ milli = parseFraction(v.substring(dot + 1, space).trim(), 100);
+ tz = TimeZone.getTimeZone(v.substring(space + 1).trim());
+ }
+ }
+ }
+ }
+ int milliOfDay = hour * (int) DateTimeUtils.MILLIS_PER_HOUR
+ + minute * (int) DateTimeUtils.MILLIS_PER_MINUTE
+ + second * (int) DateTimeUtils.MILLIS_PER_SECOND
+ + milli;
+ return new TimeWithTimeZone(milliOfDay, tz);
+ }
+
+ /** Parses a fraction, multiplying the first character by {@code multiplier},
+ * the second character by {@code multiplier / 10},
+ * the third character by {@code multiplier / 100}, and so forth.
+ *
+ * <p>For example, {@code parseFraction("1234", 100)} yields {@code 123}. */
+ private static int parseFraction(String v, int multiplier) {
+ int r = 0;
+ for (int i = 0; i < v.length(); i++) {
+ char c = v.charAt(i);
+ int x = c < '0' || c > '9' ? 0 : (c - '0');
+ r += multiplier * x;
+ if (multiplier < 10) {
+ // We're at the last digit. Check for rounding.
+ if (i + 1 < v.length()
+ && v.charAt(i + 1) >= '5') {
+ ++r;
+ }
+ break;
+ }
+ multiplier /= 10;
+ }
+ return r;
+ }
+
+
+ public static long timestampWithTimeZoneToTimestamp(TimestampWithTimeZone tstz) {
+ return tstz.getMillisecond();
+ }
+
+ public static String timestampWithTimeZoneToString(TimestampWithTimeZone tstz) {
+ String localTimestamp = DateTimeUtils.unixTimestampToString(tstz.getMillisecond());
+ return localTimestamp + " " + tstz.getTimeZone().getID();
+ }
+
+ public static TimeWithTimeZone timestampWithTimeZoneToTimeWithTimeZone(
+ TimestampWithTimeZone tstz) {
+ int milliOfDay = (int) (tstz.getMillisecond() % DateTimeUtils.MILLIS_PER_DAY);
+ return new TimeWithTimeZone(milliOfDay, tstz.getTimeZone());
+ }
+
+ public static TimestampWithTimeZone toTimestampWithTimeZone(String v, TimeZone sessionTimeZone) {
Review comment:
Maybe also rename and include `string`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361258387
##########
File path: core/src/test/java/org/apache/calcite/test/RexProgramTest.java
##########
@@ -42,14 +42,7 @@
import org.apache.calcite.sql.type.ReturnTypes;
import org.apache.calcite.sql.type.SqlTypeAssignmentRule;
import org.apache.calcite.sql.type.SqlTypeName;
-import org.apache.calcite.util.DateString;
-import org.apache.calcite.util.ImmutableBitSet;
-import org.apache.calcite.util.NlsString;
-import org.apache.calcite.util.TestUtil;
-import org.apache.calcite.util.TimeString;
-import org.apache.calcite.util.TimestampString;
-import org.apache.calcite.util.TimestampWithTimeZoneString;
-import org.apache.calcite.util.Util;
+import org.apache.calcite.util.*;
Review comment:
Don't use `*`. Import classes one by one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361258318
##########
File path: core/src/main/java/org/apache/calcite/util/TimestampWithTimeZone.java
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import java.io.Serializable;
+import java.util.Objects;
+import java.util.TimeZone;
+
+/**
+ * A Timestamp with time zone. It is immutable.
+ */
+public class TimestampWithTimeZone implements Comparable<TimestampWithTimeZone>, Serializable {
+ //~ Instance fields --------------------------------------------------------
+
+ private final long millisecond;
+ private final TimeZone timeZone;
+
+ /**
+ * Constructor. Obtains an instance of {@link TimestampWithTimeZone} from a millisecond of
+ * zoneless timestamp and a {@link TimeZone}.
+ *
+ * @param millisecond the number of milliseconds of a zoneless timestamp.
+ * @param timeZone the time-zone to use.
+ */
+ public TimestampWithTimeZone(long millisecond, TimeZone timeZone) {
+ this.millisecond = millisecond;
+ this.timeZone = timeZone;
+ }
+
+ public TimestampWithTimeZone(long millisecond, String tz) {
+ this.millisecond = millisecond;
+ this.timeZone = TimeZone.getTimeZone(tz);
+ }
+
+ /**
+ * Converts this time to epoch millis based on 1970-01-01Z.
+ *
+ * @return the epoch milli value
+ */
+ private long toEpochMilli() {
+ long nod = millisecond;
+ int offsetMillis = timeZone.getOffset(nod);
+ return nod - offsetMillis;
+ }
+
+ /**
+ * Compares this date-time to another date-time.
+ * <p>
+ * The comparison is based on the instant then on the local date-time.
+ * See {@link java.time.OffsetDateTime}
+ *
+ * @param o the other date-time to compare to.
+ * @return the comparator value, negative if less, positive if greater.
+ */
+ @Override public int compareTo(TimestampWithTimeZone o) {
+ if (timeZone.equals(o.timeZone)) {
+ return Long.compare(millisecond, o.millisecond);
+ }
+ int compare = Long.compare(toEpochMilli(), o.toEpochMilli());
+ if (compare == 0) {
+ compare = Long.compare(millisecond, o.millisecond);
+ }
+ return compare;
+ }
+
+ @Override public int hashCode() {
+ return Objects.hash(millisecond, timeZone);
+ }
+
+ @Override public boolean equals(Object obj) {
+ if (!(obj instanceof TimestampWithTimeZone)) {
+ return false;
+ }
+ TimestampWithTimeZone that = (TimestampWithTimeZone) obj;
+ return this.millisecond == that.millisecond && this.timeZone == that.timeZone;
Review comment:
`this.timeZone.equals(that.timeZone)`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361270113
##########
File path: core/src/test/java/org/apache/calcite/test/RexProgramTest.java
##########
@@ -2223,15 +2212,27 @@ private void checkExponentialCnf(int n) {
final RexLiteral literalTimeLTZ =
rexBuilder.makeTimeWithLocalTimeZoneLiteral(
new TimeString(1, 23, 45), 0);
+ final RexLiteral timeChar = rexBuilder.makeLiteral("12:34:45");
final RexLiteral timeLTZChar1 = rexBuilder.makeLiteral("12:34:45 America/Los_Angeles");
final RexLiteral timeLTZChar2 = rexBuilder.makeLiteral("12:34:45 UTC");
final RexLiteral timeLTZChar3 = rexBuilder.makeLiteral("12:34:45 GMT+01");
+ final RexLiteral timestampChar = rexBuilder.makeLiteral("2011-07-20 12:34:56");
final RexLiteral timestampLTZChar1 = rexBuilder.makeLiteral("2011-07-20 12:34:56 Asia/Tokyo");
final RexLiteral timestampLTZChar2 = rexBuilder.makeLiteral("2011-07-20 12:34:56 GMT+01");
final RexLiteral timestampLTZChar3 = rexBuilder.makeLiteral("2011-07-20 12:34:56 UTC");
final RexLiteral literalTimestampLTZ =
rexBuilder.makeTimestampWithLocalTimeZoneLiteral(
new TimestampString(2011, 7, 20, 8, 23, 45), 0);
+ final RexLiteral literalTimeWithTZ =
+ rexBuilder.makeTimeWithTimeZoneLiteral(
+ new TimeWithTimeZoneString(
+ new TimeString(1, 23, 45),
+ TimeZone.getTimeZone("Asia/Shanghai")), 0);
+ final RexLiteral literalTimestampWithTZ =
+ rexBuilder.makeTimestampWithTimeZoneLiteral(
+ new TimestampWithTimeZoneString(
+ new TimestampString(2017, 7, 20, 8, 23, 45),
Review comment:
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361570429
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java
##########
@@ -376,6 +394,17 @@ private Expression converterToTime(RelDataType sourceType, Expression operand) {
BuiltInMethod.TIMESTAMP_WITH_LOCAL_TIME_ZONE_TO_TIME.method,
operand,
Expressions.call(BuiltInMethod.TIME_ZONE.method, root)));
+ break;
+ // SQL 2011 Part2 Section 4.6 General Rules 15.e
Review comment:
Sorry, it should be Section 6.13 (begin with p243)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361551423
##########
File path: core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
##########
@@ -2021,6 +2023,137 @@ public static Long toTimestampWithLocalTimeZone(String v, TimeZone timeZone) {
.getMillisSinceEpoch();
}
+ public static int timeWithTimeZoneToTime(TimeWithTimeZone ttz) {
+ return ttz.getMilliOfDay();
+ }
+
+ public static TimestampWithTimeZone timeWithTimeZoneToTimestampWithTimeZone(int date,
+ TimeWithTimeZone ttz) {
+ long millisecond = date * DateTimeUtils.MILLIS_PER_DAY + ttz.getMilliOfDay();
+ return new TimestampWithTimeZone(millisecond, ttz.getTimeZone());
+ }
+
+ public static String timeWithTimeZoneToString(TimeWithTimeZone ttz) {
+ String localTime = DateTimeUtils.unixTimeToString(ttz.getMilliOfDay());
+ return localTime + " " + ttz.getTimeZone().getID();
+ }
+
+ public static TimeWithTimeZone toTimeWithTimeZone(String v, TimeZone sesstionTimeZone) {
Review comment:
rename to `stringToTimeWithTimeZone`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361257782
##########
File path: core/src/main/java/org/apache/calcite/rex/RexBuilder.java
##########
@@ -1345,6 +1384,10 @@ private static Comparable zeroValue(RelDataType type) {
return new TimeString(0, 0, 0);
case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
return new TimestampString(0, 0, 0, 0, 0, 0);
+ 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");
Review comment:
Shouldn't 0 is `January 1, 1970 UTC`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361588374
##########
File path: core/src/main/java/org/apache/calcite/util/TimestampWithTimeZone.java
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import java.io.Serializable;
+import java.util.Objects;
+import java.util.TimeZone;
+
+/**
+ * A Timestamp with time zone. It is immutable.
+ */
+public class TimestampWithTimeZone implements Comparable<TimestampWithTimeZone>, Serializable {
+ //~ Instance fields --------------------------------------------------------
+
+ private final long millisecond;
Review comment:
Just a long value. You can see `BuiltInMethod.STRING_TO_TIMESTAMP`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361256059
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java
##########
@@ -571,6 +323,457 @@ Expression translateCast(
return scaleIntervalToNumber(sourceType, targetType, convert);
}
+ private Expression converterToDate(RelDataType sourceType, Expression operand) {
Review comment:
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361270058
##########
File path: core/src/main/java/org/apache/calcite/util/TimestampWithTimeZone.java
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import java.io.Serializable;
+import java.util.Objects;
+import java.util.TimeZone;
+
+/**
+ * A Timestamp with time zone. It is immutable.
+ */
+public class TimestampWithTimeZone implements Comparable<TimestampWithTimeZone>, Serializable {
+ //~ Instance fields --------------------------------------------------------
+
+ private final long millisecond;
+ private final TimeZone timeZone;
+
+ /**
+ * Constructor. Obtains an instance of {@link TimestampWithTimeZone} from a millisecond of
+ * zoneless timestamp and a {@link TimeZone}.
+ *
+ * @param millisecond the number of milliseconds of a zoneless timestamp.
+ * @param timeZone the time-zone to use.
+ */
+ public TimestampWithTimeZone(long millisecond, TimeZone timeZone) {
+ this.millisecond = millisecond;
+ this.timeZone = timeZone;
+ }
+
+ public TimestampWithTimeZone(long millisecond, String tz) {
+ this.millisecond = millisecond;
+ this.timeZone = TimeZone.getTimeZone(tz);
+ }
+
+ /**
+ * Converts this time to epoch millis based on 1970-01-01Z.
+ *
+ * @return the epoch milli value
+ */
+ private long toEpochMilli() {
+ long nod = millisecond;
+ int offsetMillis = timeZone.getOffset(nod);
+ return nod - offsetMillis;
+ }
+
+ /**
+ * Compares this date-time to another date-time.
+ * <p>
+ * The comparison is based on the instant then on the local date-time.
+ * See {@link java.time.OffsetDateTime}
+ *
+ * @param o the other date-time to compare to.
+ * @return the comparator value, negative if less, positive if greater.
+ */
+ @Override public int compareTo(TimestampWithTimeZone o) {
+ if (timeZone.equals(o.timeZone)) {
+ return Long.compare(millisecond, o.millisecond);
+ }
+ int compare = Long.compare(toEpochMilli(), o.toEpochMilli());
+ if (compare == 0) {
+ compare = Long.compare(millisecond, o.millisecond);
+ }
+ return compare;
+ }
+
+ @Override public int hashCode() {
+ return Objects.hash(millisecond, timeZone);
+ }
+
+ @Override public boolean equals(Object obj) {
+ if (!(obj instanceof TimestampWithTimeZone)) {
+ return false;
+ }
+ TimestampWithTimeZone that = (TimestampWithTimeZone) obj;
+ return this.millisecond == that.millisecond && this.timeZone == that.timeZone;
Review comment:
OK
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361551079
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java
##########
@@ -376,6 +394,17 @@ private Expression converterToTime(RelDataType sourceType, Expression operand) {
BuiltInMethod.TIMESTAMP_WITH_LOCAL_TIME_ZONE_TO_TIME.method,
operand,
Expressions.call(BuiltInMethod.TIME_ZONE.method, root)));
+ break;
+ // SQL 2011 Part2 Section 4.6 General Rules 15.e
+ case TIMESTAMP_WITH_TIME_ZONE:
Review comment:
implement a `timestampWithoutTimeZoneToTimeWithoutTimeZone` and call it here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361268633
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java
##########
@@ -571,6 +323,457 @@ Expression translateCast(
return scaleIntervalToNumber(sourceType, targetType, convert);
}
+ private Expression converterToDate(RelDataType sourceType, Expression operand) {
Review comment:
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361270085
##########
File path: core/src/test/java/org/apache/calcite/test/RexProgramTest.java
##########
@@ -42,14 +42,7 @@
import org.apache.calcite.sql.type.ReturnTypes;
import org.apache.calcite.sql.type.SqlTypeAssignmentRule;
import org.apache.calcite.sql.type.SqlTypeName;
-import org.apache.calcite.util.DateString;
-import org.apache.calcite.util.ImmutableBitSet;
-import org.apache.calcite.util.NlsString;
-import org.apache.calcite.util.TestUtil;
-import org.apache.calcite.util.TimeString;
-import org.apache.calcite.util.TimestampString;
-import org.apache.calcite.util.TimestampWithTimeZoneString;
-import org.apache.calcite.util.Util;
+import org.apache.calcite.util.*;
Review comment:
OK
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361551249
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java
##########
@@ -376,6 +394,17 @@ private Expression converterToTime(RelDataType sourceType, Expression operand) {
BuiltInMethod.TIMESTAMP_WITH_LOCAL_TIME_ZONE_TO_TIME.method,
operand,
Expressions.call(BuiltInMethod.TIME_ZONE.method, root)));
+ break;
+ // SQL 2011 Part2 Section 4.6 General Rules 15.e
Review comment:
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361579628
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java
##########
@@ -341,6 +344,16 @@ private Expression converterToDate(RelDataType sourceType, Expression operand) {
BuiltInMethod.TIMESTAMP_WITH_LOCAL_TIME_ZONE_TO_DATE.method,
operand,
Expressions.call(BuiltInMethod.TIME_ZONE.method, root)));
+ break;
+ // SQL 2011 Part2 Section 4.6 General Rules 13.d
+ case TIMESTAMP_WITH_TIME_ZONE:
+ convert = Expressions.convert_(
+ Expressions.call(BuiltInMethod.FLOOR_DIV.method,
Review comment:
Not needed!
1) Section 6.13 General Rules 13.d shows this can be computed as "CAST ( CAST ( VE AS TIMESTAMP WITHOUT TIME ZONE ) AS DATE )"
2) the underlying TimestampWithTimeZone holds the same long value with Timestamp type, current implementation is more simple
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1687:
[CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#discussion_r361550966
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java
##########
@@ -341,6 +344,16 @@ private Expression converterToDate(RelDataType sourceType, Expression operand) {
BuiltInMethod.TIMESTAMP_WITH_LOCAL_TIME_ZONE_TO_DATE.method,
operand,
Expressions.call(BuiltInMethod.TIME_ZONE.method, root)));
+ break;
+ // SQL 2011 Part2 Section 4.6 General Rules 13.d
+ case TIMESTAMP_WITH_TIME_ZONE:
+ convert = Expressions.convert_(
+ Expressions.call(BuiltInMethod.FLOOR_DIV.method,
Review comment:
implement a `timestampWithoutTimeZoneToDate` and call it here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] docete commented on issue #1687: [CALCITE-3612] Add
TIME/TIMESTAMP WITH TIME ZONE in optimizer
Posted by GitBox <gi...@apache.org>.
docete commented on issue #1687: [CALCITE-3612] Add TIME/TIMESTAMP WITH TIME ZONE in optimizer
URL: https://github.com/apache/calcite/pull/1687#issuecomment-568869728
rebased to split the refactoring to a separate commit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services