[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r346109995
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##
@@ -174,8 +174,8 @@ object ScalarOperatorGens {
(l, r) => s"$l $op ((int) ($r / ${MILLIS_PER_DAY}L))"
}
case TIMESTAMP_WITHOUT_TIME_ZONE =>
-generateOperatorIfNotNull(ctx, new TimestampType(), left, right) {
- (l, r) => s"($l * ${MILLIS_PER_DAY}L) $op $r"
+generateOperatorIfNotNull(ctx, new TimestampType(3), left, right) {
Review comment:
OK, should use resultType 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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r346092808
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/GenerateUtils.scala
##
@@ -370,8 +369,17 @@ object GenerateUtils {
generateNonNullLiteral(literalType, literalValue.toString,
literalValue)
case TIMESTAMP_WITHOUT_TIME_ZONE =>
-val millis = literalValue.asInstanceOf[Long]
-generateNonNullLiteral(literalType, millis + "L", millis)
+val fieldTerm = newName("timestamp")
+val timestampString =
literalValue.asInstanceOf[TimestampString].toString
+val millis = getMillisSinceEpoch(timestampString)
Review comment:
`getMillisSinceEpoch` is a util function of `TimestampString`, we copied it
because our copied `DateTimeUtils` causes Gregorian cutovers issues. We should
use `TimestampString.getMillisSinceEpoch` directly when we remove our copied
`DateTimeUtils`. Do you mean we should not depend on `TimestampString` and
parse the timestamp string by ourself?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r345724495
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarFunctionCallGen.scala
##
@@ -66,7 +69,15 @@ class ScalarFunctionCallGen(scalarFunction: ScalarFunction)
extends CallGenerato
boxedTypeTermForType(returnType)
}
val resultTerm = ctx.addReusableLocalVariable(resultTypeTerm, "result")
-val evalResult =
s"$functionReference.eval(${parameters.map(_.resultTerm).mkString(", ")})"
+val evalResult =
+ if (returnType.getTypeRoot ==
LogicalTypeRoot.TIMESTAMP_WITHOUT_TIME_ZONE) {
Review comment:
I have no idea whether we should disable a UDX with `long` parameter to
accept a `Timestamp`. What do you think @KurtYoung ?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r345708001
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/GenerateUtils.scala
##
@@ -370,8 +369,17 @@ object GenerateUtils {
generateNonNullLiteral(literalType, literalValue.toString,
literalValue)
case TIMESTAMP_WITHOUT_TIME_ZONE =>
-val millis = literalValue.asInstanceOf[Long]
-generateNonNullLiteral(literalType, millis + "L", millis)
+val fieldTerm = newName("timestamp")
+val timestampString =
literalValue.asInstanceOf[TimestampString].toString
+val millis = getMillisSinceEpoch(timestampString)
Review comment:
How can `TimestampString` get a `LocalDateTime`?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r345706658
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/ExpressionReducer.scala
##
@@ -172,6 +172,19 @@ class ExpressionReducer(
}
reducedValues.add(maySkipNullLiteralReduce(rexBuilder, value,
unreduced))
reducedIdx += 1
+ case SqlTypeName.TIMESTAMP =>
+val reducedValue = reduced.getField(reducedIdx)
+val value = if (reducedValue != null) {
+ val ts = reducedValue.asInstanceOf[SqlTimestamp]
+ val milliseconds = ts.getMillisecond
+ val nanoseconds = ts.toLocalDateTime.getNano
Review comment:
A little tricky here, take `CAST('1500-04-30 00:00:00' AS TIMESTAMP(3))` as
an example:
we use `SqlDateTimeUtils.toTimestamp`[1] to cast the string to SqlTimestamp
and use `ExpressionReducer`[2] to make a Timestamp literal.
The [1] step considers Gregorian cutovers and again the [2] step uses
`TimestampString.fromMillisSinceEpoch` which calls our copied
`DateTimeUtils.julianToString` considers Gregorian cutovers too. Two wrongs
make a correct result.
I will change the `toTimestamp` and this `ExpressionReducer` to ignore
Gregorian cutovers in next PR and that will be the final 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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r345703849
##
File path:
flink-table/flink-table-planner-blink/src/test/scala/org/apache/flink/table/planner/expressions/DateTimeTypesTest.scala
##
@@ -177,14 +193,14 @@ class TemporalTypesTest extends ExpressionTestBase {
testAllApis(
'f0.cast(DataTypes.TIMESTAMP(3)),
"f0.cast(SQL_TIMESTAMP)",
- "CAST(f0 AS TIMESTAMP)",
- "1990-10-14 00:00:00.000")
+ "CAST(f0 AS TIMESTAMP(3))",
+ "1990-10-14 00:00:00")
Review comment:
No. It just describe the cast specification from datetime type to
char/varchar type:
`If SD is a datetime data type or an interval data type then let Y be the
shortest character string that
conforms to the definition of in Subclause 5.3, “”, and
such that the interpreted value
of Y is SV and the interpreted precision of Y is the precision of SD. If SV
is a negative interval, then
shall be specified within in the literal
Y.`
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r345624549
##
File path:
flink-table/flink-table-planner-blink/src/test/scala/org/apache/flink/table/planner/expressions/DateTimeTypesTest.scala
##
@@ -177,14 +193,14 @@ class TemporalTypesTest extends ExpressionTestBase {
testAllApis(
'f0.cast(DataTypes.TIMESTAMP(3)),
"f0.cast(SQL_TIMESTAMP)",
- "CAST(f0 AS TIMESTAMP)",
- "1990-10-14 00:00:00.000")
+ "CAST(f0 AS TIMESTAMP(3))",
+ "1990-10-14 00:00:00")
Review comment:
Ignore the tailing '0's is more standard-compliant since it's the shortest
to represent the literal?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r345604974
##
File path:
flink-table/flink-table-planner-blink/src/test/scala/org/apache/flink/table/planner/expressions/DateTimeTypesTest.scala
##
@@ -177,14 +193,14 @@ class TemporalTypesTest extends ExpressionTestBase {
testAllApis(
'f0.cast(DataTypes.TIMESTAMP(3)),
"f0.cast(SQL_TIMESTAMP)",
- "CAST(f0 AS TIMESTAMP)",
- "1990-10-14 00:00:00.000")
+ "CAST(f0 AS TIMESTAMP(3))",
+ "1990-10-14 00:00:00")
Review comment:
SQL 2011 defined the interpreted string value as `the shortest character
string that conforms to the definition of literal`. And I checked other DBMSs:
MySQL 5.6: 'SELECT CAST(TIMESTAMP '1970-01-01 11:22:33' AS DATETIME(3))' =>
'1970-01-01T11:22:33Z'
PG 9.6: `SELECT CAST('1970-01-01 11:22:33' AS TIMESTAMP(3))' =>
'1970-01-01T11:22:33Z'
ORACLE 11g R2: `select CAST(TIMESTAMP '1970-01-01 11:22:33' AS TIMESTAMP(3))
from log_table` => '1970-01-01 11:22:33.0'
MSSQL 2017: `SELECT CAST({ts '1970-01-01 11:22:33.000'} AS DATETIME),
CAST({ts '1970-01-01 11:22:33.000'} AS DATETIME2)` => '1970-01-01T11:22:33Z',
'1970-01-01 11:22:33.000'
Only MSSQL's DATETIME2 will preserve tailing '0's and the precision can not
be specified. IMO we shouldn't preserve the tailing '0's.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r345614091
##
File path: flink-table/flink-table-runtime-blink/pom.xml
##
@@ -74,6 +74,67 @@ under the License.
${janino.version}
+
+
+ org.apache.calcite
+ calcite-core
Review comment:
Is there any harmful things to introduce calcite dependency?
The calcite's TimestampString provides standard-compliant processing of
Timestamp literal which we can depend on.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r345604974
##
File path:
flink-table/flink-table-planner-blink/src/test/scala/org/apache/flink/table/planner/expressions/DateTimeTypesTest.scala
##
@@ -177,14 +193,14 @@ class TemporalTypesTest extends ExpressionTestBase {
testAllApis(
'f0.cast(DataTypes.TIMESTAMP(3)),
"f0.cast(SQL_TIMESTAMP)",
- "CAST(f0 AS TIMESTAMP)",
- "1990-10-14 00:00:00.000")
+ "CAST(f0 AS TIMESTAMP(3))",
+ "1990-10-14 00:00:00")
Review comment:
MySQL 5.6: 'SELECT CAST(TIMESTAMP '1970-01-01 11:22:33' AS DATETIME(3))' =>
'1970-01-01T11:22:33Z'
PG 9.6: `SELECT CAST('1970-01-01 11:22:33' AS TIMESTAMP(3))' =>
'1970-01-01T11:22:33Z'
ORACLE: `select CAST(TIMESTAMP '1970-01-01 11:22:33' AS TIMESTAMP(3)) from
log_table` => '1970-01-01 11:22:33.0'
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r345540542
##
File path:
flink-table/flink-table-planner-blink/src/test/scala/org/apache/flink/table/planner/expressions/ArrayTypeTest.scala
##
@@ -94,6 +96,48 @@ class ArrayTypeTest extends ArrayTypeTestBase {
"ARRAY[TIMESTAMP '1985-04-11 14:15:16', TIMESTAMP '2018-07-26
17:18:19']",
"[1985-04-11 14:15:16.000, 2018-07-26 17:18:19.000]")
+// localDateTime use DateTimeUtils.timestampStringToUnixDate to parse a
time string,
+// which only support millisecond's precision.
+testTableApi(
+ Array(
+JLocalDateTime.of(1985, 4, 11, 14, 15, 16, 123456789),
+JLocalDateTime.of(2018, 7, 26, 17, 18, 19, 123456789)),
+"[1985-04-11T14:15:16.123456789, 2018-07-26T17:18:19.123456789]")
+
+testTableApi(
+ Array(
+JLocalDateTime.of(1985, 4, 11, 14, 15, 16, 123456700),
+JLocalDateTime.of(2018, 7, 26, 17, 18, 19, 123456700)),
+ "[1985-04-11T14:15:16.123456700, 2018-07-26T17:18:19.123456700]")
+
+testTableApi(
+ Array(
+JLocalDateTime.of(1985, 4, 11, 14, 15, 16, 123456000),
+JLocalDateTime.of(2018, 7, 26, 17, 18, 19, 123456000)),
+ "[1985-04-11T14:15:16.123456, 2018-07-26T17:18:19.123456]")
+
+testTableApi(
+ Array(
+JLocalDateTime.of(1985, 4, 11, 14, 15, 16, 12340),
+JLocalDateTime.of(2018, 7, 26, 17, 18, 19, 12340)),
+ "[1985-04-11T14:15:16.123400, 2018-07-26T17:18:19.123400]")
+
+testSqlApi(
+ "ARRAY[TIMESTAMP '1985-04-11 14:15:16.123456789', TIMESTAMP '2018-07-26
17:18:19.123456789']",
+ "[1985-04-11T14:15:16.123456789, 2018-07-26T17:18:19.123456789]")
+
+testSqlApi(
+ "ARRAY[TIMESTAMP '1985-04-11 14:15:16.1234567', TIMESTAMP '2018-07-26
17:18:19.1234567']",
+ "[1985-04-11T14:15:16.123456700, 2018-07-26T17:18:19.123456700]")
Review comment:
The interpreted string should conforms to the definition of timestamp
literal(SQL 2011 Part 2 Section 6.13 General Rules (11) (d)). And i will fix it
in next 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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r345539513
##
File path:
flink-table/flink-table-planner-blink/src/test/scala/org/apache/flink/table/planner/expressions/TemporalTypesTest.scala
##
@@ -37,6 +37,17 @@ import java.util.{Locale, TimeZone}
class TemporalTypesTest extends ExpressionTestBase {
Review comment:
Double checked the `TemporalTypesTest` and find some datetime function can
handle multiple datetime types(e.g. extract/floor etc). Split the class causes
tests for one single function live in different places. So I would rename this
as DateTimeTypesTest to avoid confusing.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r345041757
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/calcite/FlinkTypeSystem.scala
##
@@ -53,6 +53,9 @@ class FlinkTypeSystem extends RelDataTypeSystemImpl {
case SqlTypeName.VARCHAR | SqlTypeName.CHAR | SqlTypeName.VARBINARY |
SqlTypeName.BINARY =>
Int.MaxValue
+// The maximal precision of TIMESTAMP is 3, change it to 9 to support
nanoseconds precision
+case SqlTypeName.TIMESTAMP => 9
Review comment:
change it 6 and fix failures in 90f33163
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344581143
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/GenerateUtils.scala
##
@@ -370,8 +373,56 @@ object GenerateUtils {
generateNonNullLiteral(literalType, literalValue.toString,
literalValue)
case TIMESTAMP_WITHOUT_TIME_ZONE =>
-val millis = literalValue.asInstanceOf[Long]
-generateNonNullLiteral(literalType, millis + "L", millis)
+def getNanoOfMillisSinceEpoch(timestampString: TimestampString): Int =
{
+ val v = timestampString.toString()
+ val length = v.length
+ val nanoOfSeconds = length match {
+case 19 | 20 => 0
+case _ =>
+ JInteger.valueOf(v.substring(20)) * pow(10, 9 - (length -
20)).intValue()
+ }
+ nanoOfSeconds % 100
+}
+
+// TODO: we copied the logical of TimestampString::getMillisSinceEpoch
since the copied
+// DateTimeUtils.ymdToJulian is wrong.
Review comment:
[FLINK-11935](https://issues.apache.org/jira/browse/FLINK-11935) should do
this, and after that this copied code could be removed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344580399
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/GenerateUtils.scala
##
@@ -370,8 +373,56 @@ object GenerateUtils {
generateNonNullLiteral(literalType, literalValue.toString,
literalValue)
case TIMESTAMP_WITHOUT_TIME_ZONE =>
-val millis = literalValue.asInstanceOf[Long]
-generateNonNullLiteral(literalType, millis + "L", millis)
+def getNanoOfMillisSinceEpoch(timestampString: TimestampString): Int =
{
+ val v = timestampString.toString()
+ val length = v.length
+ val nanoOfSeconds = length match {
+case 19 | 20 => 0
+case _ =>
+ JInteger.valueOf(v.substring(20)) * pow(10, 9 - (length -
20)).intValue()
+ }
+ nanoOfSeconds % 100
+}
+
+// TODO: we copied the logical of TimestampString::getMillisSinceEpoch
since the copied
+// DateTimeUtils.ymdToJulian is wrong.
Review comment:
Two reasons to not fixing our copied `DateTimeUtils` in this PR
1) two copies of `DateTimeUtils` should remain the same in legacy planner
and blink planner
2) and the impact should be evaluated for both planner
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344549947
##
File path:
flink-table/flink-table-runtime-blink/src/main/java/org/apache/flink/table/dataformat/BinaryArray.java
##
@@ -191,6 +191,22 @@ public Decimal getDecimal(int pos, int precision, int
scale) {
return Decimal.readDecimalFieldFromSegments(segments, offset,
offsetAndSize, precision, scale);
}
+ @Override
+ public SqlTimestamp getTimestamp(int pos, int precision) {
+ assertIndexIsValid(pos);
+
+ if (SqlTimestamp.isCompact(precision)) {
+ return
SqlTimestamp.fromEpochMillis(segments[0].getLong(getElementOffset(pos, 8)));
Review comment:
Good catch, it should be `SegmentsUtil.getLong(segments,
getElementOffset(pos, 8)`
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344549642
##
File path:
flink-table/flink-table-planner-blink/src/test/scala/org/apache/flink/table/planner/expressions/TemporalTypesTest.scala
##
@@ -37,6 +37,17 @@ import java.util.{Locale, TimeZone}
class TemporalTypesTest extends ExpressionTestBase {
Review comment:
So divide it to TimestampTypeTest/DateTypeTest/TimeTypeTest ?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner URL: https://github.com/apache/flink/pull/10105#discussion_r344549396 ## File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/plan/utils/RexNodeExtractor.scala ## @@ -335,7 +336,7 @@ class RexNodeToExpressionConverter( case TIMESTAMP_WITHOUT_TIME_ZONE => val v = literal.getValueAs(classOf[java.lang.Long]) Review comment: Good catch. It should be TimestampString and preserve the nanosecond precision. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344548144
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##
@@ -2184,7 +2236,20 @@ object ScalarOperatorGens {
case TIME_WITHOUT_TIME_ZONE =>
s"${qualifyMethod(BuiltInMethods.UNIX_TIME_TO_STRING)}($operandTerm)"
case TIMESTAMP_WITHOUT_TIME_ZONE => // including rowtime indicator
-s"${qualifyMethod(BuiltInMethods.TIMESTAMP_TO_STRING)}($operandTerm,
3)"
+// casting TimestampType to VARCHAR, if precision <= 3, keep the
string representation
Review comment:
Just keep consistent with the original design. Or the behavior is changed
from the users' side. Should we change this in this 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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344547747
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##
@@ -174,8 +174,8 @@ object ScalarOperatorGens {
(l, r) => s"$l $op ((int) ($r / ${MILLIS_PER_DAY}L))"
}
case TIMESTAMP_WITHOUT_TIME_ZONE =>
-generateOperatorIfNotNull(ctx, new TimestampType(), left, right) {
- (l, r) => s"($l * ${MILLIS_PER_DAY}L) $op $r"
+generateOperatorIfNotNull(ctx, new TimestampType(3), left, right) {
Review comment:
The original `new TimestampType()` just returns `TimestampType(3)`. And just
keep consistent with the original implementation. I will confirm the right
behavior with Jack and Danny Chan and Fix it in the next PR
[FLINK-14696](https://issues.apache.org/jira/browse/FLINK-14696)
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344547802
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##
@@ -1073,51 +1124,51 @@ object ScalarOperatorGens {
(INTEGER, TIMESTAMP_WITHOUT_TIME_ZONE) |
(BIGINT, TIMESTAMP_WITHOUT_TIME_ZONE) =>
generateUnaryOperatorIfNotNull(ctx, targetType, operand) {
-operandTerm => s"(((long) $operandTerm) * 1000)"
+operandTerm => s"$SQL_TIMESTAMP.fromEpochMillis(((long) $operandTerm)
* 1000)"
}
// Float -> Timestamp
// Double -> Timestamp
case (FLOAT, TIMESTAMP_WITHOUT_TIME_ZONE) |
(DOUBLE, TIMESTAMP_WITHOUT_TIME_ZONE) =>
generateUnaryOperatorIfNotNull(ctx, targetType, operand) {
-operandTerm => s"((long) ($operandTerm * 1000))"
+operandTerm => s"$SQL_TIMESTAMP.fromEpochMillis((long) ($operandTerm *
1000))"
}
// Timestamp -> Tinyint
case (TIMESTAMP_WITHOUT_TIME_ZONE, TINYINT) =>
generateUnaryOperatorIfNotNull(ctx, targetType, operand) {
-operandTerm => s"((byte) ($operandTerm / 1000))"
+operandTerm => s"((byte) ($operandTerm.getMillisecond() / 1000))"
}
// Timestamp -> Smallint
case (TIMESTAMP_WITHOUT_TIME_ZONE, SMALLINT) =>
generateUnaryOperatorIfNotNull(ctx, targetType, operand) {
-operandTerm => s"((short) ($operandTerm / 1000))"
+operandTerm => s"((short) ($operandTerm.getMillisecond() / 1000))"
}
// Timestamp -> Int
case (TIMESTAMP_WITHOUT_TIME_ZONE, INTEGER) =>
generateUnaryOperatorIfNotNull(ctx, targetType, operand) {
-operandTerm => s"((int) ($operandTerm / 1000))"
+operandTerm => s"((int) ($operandTerm.getMillisecond() / 1000))"
}
// Timestamp -> BigInt
case (TIMESTAMP_WITHOUT_TIME_ZONE, BIGINT) =>
generateUnaryOperatorIfNotNull(ctx, targetType, operand) {
-operandTerm => s"((long) ($operandTerm / 1000))"
+operandTerm => s"((long) ($operandTerm.getMillisecond() / 1000))"
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344543636
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarFunctionCallGen.scala
##
@@ -66,7 +69,15 @@ class ScalarFunctionCallGen(scalarFunction: ScalarFunction)
extends CallGenerato
boxedTypeTermForType(returnType)
}
val resultTerm = ctx.addReusableLocalVariable(resultTypeTerm, "result")
-val evalResult =
s"$functionReference.eval(${parameters.map(_.resultTerm).mkString(", ")})"
+val evalResult =
+ if (returnType.getTypeRoot ==
LogicalTypeRoot.TIMESTAMP_WITHOUT_TIME_ZONE) {
Review comment:
Currently many function use Long as the internal representation of
Timestamp. And the document recommend this usage, see [Types.TIMESTAMP can be
represented as
long](https://ci.apache.org/projects/flink/flink-docs-release-1.9/dev/table/udfs.html#best-practices-for-implementing-udfs).
We should cover this.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344542341
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/GenerateUtils.scala
##
@@ -370,8 +373,56 @@ object GenerateUtils {
generateNonNullLiteral(literalType, literalValue.toString,
literalValue)
case TIMESTAMP_WITHOUT_TIME_ZONE =>
-val millis = literalValue.asInstanceOf[Long]
-generateNonNullLiteral(literalType, millis + "L", millis)
+def getNanoOfMillisSinceEpoch(timestampString: TimestampString): Int =
{
+ val v = timestampString.toString()
+ val length = v.length
+ val nanoOfSeconds = length match {
+case 19 | 20 => 0
+case _ =>
+ JInteger.valueOf(v.substring(20)) * pow(10, 9 - (length -
20)).intValue()
+ }
+ nanoOfSeconds % 100
+}
+
+// TODO: we copied the logical of TimestampString::getMillisSinceEpoch
since the copied
+// DateTimeUtils.ymdToJulian is wrong.
+// SEE CALCITE-1884
+def getMillisInSecond(timestampString: TimestampString): Int = {
+ val v = timestampString.toString()
+ val length = v.length
+ val milliOfSeconds = length match {
+case 19 => 0
+case 21 => JInteger.valueOf(v.substring(20)).intValue() * 100
+case 22 => JInteger.valueOf(v.substring(20)).intValue() * 10
+case 20 | 23 | _ => JInteger.valueOf(v.substring(20,
23)).intValue()
+ }
+ milliOfSeconds
+}
+
+def getMillisSinceEpoch(timestampString: TimestampString): Long = {
+ val v = timestampString.toString()
+ val year = JInteger.valueOf(v.substring(0, 4))
+ val month = JInteger.valueOf(v.substring(5, 7))
+ val day = JInteger.valueOf(v.substring(8, 10))
+ val h = JInteger.valueOf(v.substring(11, 13))
+ val m = JInteger.valueOf(v.substring(14, 16))
+ val s = JInteger.valueOf(v.substring(17, 19))
+ val ms = getMillisInSecond(timestampString)
+ val d = SqlDateTimeUtils.ymdToJulian(year, month, day)
+ d * 8640L + h * 360L + m * 6L + s * 1000L + ms.toLong
+}
+
+val fieldTerm = newName("timestamp")
+val millis =
literalValue.asInstanceOf[TimestampString].getMillisSinceEpoch
Review comment:
The timestamp literals in SQL text has passed to a TimestampString instant
of a Long(for preserving precision information). See
ExprCodeGenerator.scala::visitLiteral (line 389):
`
override def visitLiteral(literal: RexLiteral): GeneratedExpression = {
val resultType = FlinkTypeFactory.toLogicalType(literal.getType)
val value = resultType.getTypeRoot match {
case LogicalTypeRoot.TIMESTAMP_WITHOUT_TIME_ZONE =>
literal.getValueAs(classOf[TimestampString])
case _ =>
literal.getValue3
}
generateLiteral(ctx, resultType, 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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344540487
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/GenerateUtils.scala
##
@@ -370,8 +373,56 @@ object GenerateUtils {
generateNonNullLiteral(literalType, literalValue.toString,
literalValue)
case TIMESTAMP_WITHOUT_TIME_ZONE =>
-val millis = literalValue.asInstanceOf[Long]
-generateNonNullLiteral(literalType, millis + "L", millis)
+def getNanoOfMillisSinceEpoch(timestampString: TimestampString): Int =
{
+ val v = timestampString.toString()
+ val length = v.length
+ val nanoOfSeconds = length match {
+case 19 | 20 => 0
+case _ =>
+ JInteger.valueOf(v.substring(20)) * pow(10, 9 - (length -
20)).intValue()
+ }
+ nanoOfSeconds % 100
+}
+
+// TODO: we copied the logical of TimestampString::getMillisSinceEpoch
since the copied
+// DateTimeUtils.ymdToJulian is wrong.
Review comment:
Yes. CALCITE-1884 is not fixed in our copied DateTimeUtils. And it's the
root cause of some delicate cases, such as:
`
SELECT TIMESTAMP '1500-04-30 00:00:00.123456789' FROM docs;
SELECT CAST('1500-04-30 00:00:00.123456789' AS DATETIME(9)) FROM docs;
`
should returns
`
1500-05-10T00:00:00.123456789
`
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344539915
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/calcite/FlinkTypeSystem.scala
##
@@ -53,6 +53,9 @@ class FlinkTypeSystem extends RelDataTypeSystemImpl {
case SqlTypeName.VARCHAR | SqlTypeName.CHAR | SqlTypeName.VARBINARY |
SqlTypeName.BINARY =>
Int.MaxValue
+// The maximal precision of TIMESTAMP is 3, change it to 9 to support
nanoseconds precision
+case SqlTypeName.TIMESTAMP => 9
Review comment:
Yes. But the default precision of Timestamp in Calcite is 3.
Should we change this behavior?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344539656
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/calcite/FlinkTypeFactory.scala
##
@@ -424,11 +424,12 @@ object FlinkTypeFactory {
// blink runner support precision 3, but for consistent with flink
runner, we set to 0.
new TimeType()
case TIMESTAMP =>
-if (relDataType.getPrecision > 3) {
+val precision = relDataType.getPrecision
+if (precision > 9 || precision < 0) {
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344539605
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/calcite/FlinkTypeFactory.scala
##
@@ -424,11 +424,12 @@ object FlinkTypeFactory {
// blink runner support precision 3, but for consistent with flink
runner, we set to 0.
new TimeType()
case TIMESTAMP =>
-if (relDataType.getPrecision > 3) {
+val precision = relDataType.getPrecision
+if (precision > 9 || precision < 0) {
throw new TableException(
-s"TIMESTAMP precision is not supported:
${relDataType.getPrecision}")
+s"TIMESTAMP precision is not supported: ${precision}")
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344539546
##
File path:
flink-table/flink-table-planner-blink/src/main/java/org/apache/flink/table/planner/expressions/converter/ExpressionConverter.java
##
@@ -135,8 +138,26 @@ public RexNode visit(ValueLiteralExpression valueLiteral)
{
return
relBuilder.getRexBuilder().makeTimeLiteral(TimeString.fromCalendarFields(
valueAsCalendar(extractValue(valueLiteral, java.sql.Time.class))), 0);
case TIMESTAMP_WITHOUT_TIME_ZONE:
- return
relBuilder.getRexBuilder().makeTimestampLiteral(TimestampString.fromCalendarFields(
-
valueAsCalendar(extractValue(valueLiteral, java.sql.Timestamp.class))), 3);
+ TimestampType timestampType = (TimestampType)
type;
+ Class clazz =
valueLiteral.getOutputDataType().getConversionClass();
+ LocalDateTime datetime = null;
+ if (clazz == LocalDateTime.class) {
+ datetime =
valueLiteral.getValueAs(LocalDateTime.class)
+ .orElseThrow(() -> new
TableException("Invalid literal."));
+ } else if (clazz == Timestamp.class) {
+ datetime =
valueLiteral.getValueAs(Timestamp.class)
+ .orElseThrow(() -> new
TableException("Invalid literal.")).toLocalDateTime();
+ } else {
+ throw new TableException("Invalid
literal.");
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344539268
##
File path:
flink-table/flink-table-planner-blink/src/test/scala/org/apache/flink/table/planner/expressions/ArrayTypeTest.scala
##
@@ -94,6 +96,48 @@ class ArrayTypeTest extends ArrayTypeTestBase {
"ARRAY[TIMESTAMP '1985-04-11 14:15:16', TIMESTAMP '2018-07-26
17:18:19']",
"[1985-04-11 14:15:16.000, 2018-07-26 17:18:19.000]")
+// localDateTime use DateTimeUtils.timestampStringToUnixDate to parse a
time string,
+// which only support millisecond's precision.
+testTableApi(
+ Array(
+JLocalDateTime.of(1985, 4, 11, 14, 15, 16, 123456789),
+JLocalDateTime.of(2018, 7, 26, 17, 18, 19, 123456789)),
+"[1985-04-11T14:15:16.123456789, 2018-07-26T17:18:19.123456789]")
+
+testTableApi(
+ Array(
+JLocalDateTime.of(1985, 4, 11, 14, 15, 16, 123456700),
+JLocalDateTime.of(2018, 7, 26, 17, 18, 19, 123456700)),
+ "[1985-04-11T14:15:16.123456700, 2018-07-26T17:18:19.123456700]")
+
+testTableApi(
+ Array(
+JLocalDateTime.of(1985, 4, 11, 14, 15, 16, 123456000),
+JLocalDateTime.of(2018, 7, 26, 17, 18, 19, 123456000)),
+ "[1985-04-11T14:15:16.123456, 2018-07-26T17:18:19.123456]")
+
+testTableApi(
+ Array(
+JLocalDateTime.of(1985, 4, 11, 14, 15, 16, 12340),
+JLocalDateTime.of(2018, 7, 26, 17, 18, 19, 12340)),
+ "[1985-04-11T14:15:16.123400, 2018-07-26T17:18:19.123400]")
+
+testSqlApi(
+ "ARRAY[TIMESTAMP '1985-04-11 14:15:16.123456789', TIMESTAMP '2018-07-26
17:18:19.123456789']",
+ "[1985-04-11T14:15:16.123456789, 2018-07-26T17:18:19.123456789]")
+
+testSqlApi(
+ "ARRAY[TIMESTAMP '1985-04-11 14:15:16.1234567', TIMESTAMP '2018-07-26
17:18:19.1234567']",
+ "[1985-04-11T14:15:16.123456700, 2018-07-26T17:18:19.123456700]")
+
+testSqlApi(
+ "ARRAY[TIMESTAMP '1985-04-11 14:15:16.123456', TIMESTAMP '2018-07-26
17:18:19.123456']",
+ "[1985-04-11T14:15:16.123456, 2018-07-26T17:18:19.123456]")
+
+testSqlApi(
+ "ARRAY[TIMESTAMP '1985-04-11 14:15:16.1234', TIMESTAMP '2018-07-26
17:18:19.1234']",
+ "[1985-04-11T14:15:16.123400, 2018-07-26T17:18:19.123400]")
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r344539204
##
File path:
flink-table/flink-table-planner-blink/src/test/scala/org/apache/flink/table/planner/expressions/ArrayTypeTest.scala
##
@@ -94,6 +96,48 @@ class ArrayTypeTest extends ArrayTypeTestBase {
"ARRAY[TIMESTAMP '1985-04-11 14:15:16', TIMESTAMP '2018-07-26
17:18:19']",
"[1985-04-11 14:15:16.000, 2018-07-26 17:18:19.000]")
+// localDateTime use DateTimeUtils.timestampStringToUnixDate to parse a
time string,
+// which only support millisecond's precision.
+testTableApi(
+ Array(
+JLocalDateTime.of(1985, 4, 11, 14, 15, 16, 123456789),
+JLocalDateTime.of(2018, 7, 26, 17, 18, 19, 123456789)),
+"[1985-04-11T14:15:16.123456789, 2018-07-26T17:18:19.123456789]")
+
+testTableApi(
+ Array(
+JLocalDateTime.of(1985, 4, 11, 14, 15, 16, 123456700),
+JLocalDateTime.of(2018, 7, 26, 17, 18, 19, 123456700)),
+ "[1985-04-11T14:15:16.123456700, 2018-07-26T17:18:19.123456700]")
+
+testTableApi(
+ Array(
+JLocalDateTime.of(1985, 4, 11, 14, 15, 16, 123456000),
+JLocalDateTime.of(2018, 7, 26, 17, 18, 19, 123456000)),
+ "[1985-04-11T14:15:16.123456, 2018-07-26T17:18:19.123456]")
+
+testTableApi(
+ Array(
+JLocalDateTime.of(1985, 4, 11, 14, 15, 16, 12340),
+JLocalDateTime.of(2018, 7, 26, 17, 18, 19, 12340)),
+ "[1985-04-11T14:15:16.123400, 2018-07-26T17:18:19.123400]")
+
+testSqlApi(
+ "ARRAY[TIMESTAMP '1985-04-11 14:15:16.123456789', TIMESTAMP '2018-07-26
17:18:19.123456789']",
+ "[1985-04-11T14:15:16.123456789, 2018-07-26T17:18:19.123456789]")
+
+testSqlApi(
+ "ARRAY[TIMESTAMP '1985-04-11 14:15:16.1234567', TIMESTAMP '2018-07-26
17:18:19.1234567']",
+ "[1985-04-11T14:15:16.123456700, 2018-07-26T17:18:19.123456700]")
Review comment:
We use the LocalDateTime.toString style to represent string-format
TimestampType when precision > 3. It follows one of the following ISO-8601
formats:
-MM-dd'T'HH:mm:ss.SS
-MM-dd'T'HH:mm:ss.S
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r343607640
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/GenerateUtils.scala
##
@@ -455,13 +465,15 @@ object GenerateUtils {
def generateRowtimeAccess(
ctx: CodeGeneratorContext,
contextTerm: String): GeneratedExpression = {
+val resultType = new TimestampType(true, TimestampKind.ROWTIME, 3)
Review comment:
This is used for `STREAMRECORD_TIMESTAMP` and `MATCH_ROWTIME`. The first one
is used to access to timestamp of a `StreamRecord` which is a long value. The
second one is used to access an event time attribute from MATCH_RECOGNIZE which
is also a long value.
They use `ProcessFunction.Context`.`timestamp()` to get the underlying long
value.
I will change this function name to `generateTimestampAccess` to avoid
confusing.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r343553591
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/ExpressionReducer.scala
##
@@ -172,6 +170,15 @@ class ExpressionReducer(
}
reducedValues.add(maySkipNullLiteralReduce(rexBuilder, value,
unreduced))
reducedIdx += 1
+ case SqlTypeName.TIMESTAMP =>
+val reducedValue = reduced.getField(reducedIdx)
+val value = if (reducedValue != null) {
+ Long.box(reducedValue.asInstanceOf[SqlTimestamp].getMillisecond)
Review comment:
Just want not break the original sql timestamp literal logic.
I think an individual commit would catch this, and let the sql timestamp
literal support precision.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r343554982
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/GenerateUtils.scala
##
@@ -455,13 +465,15 @@ object GenerateUtils {
def generateRowtimeAccess(
ctx: CodeGeneratorContext,
contextTerm: String): GeneratedExpression = {
+val resultType = new TimestampType(true, TimestampKind.ROWTIME, 3)
Review comment:
IMO rowtime and proctime should be Timestamp(3) and keep millisecond
precision.
Is it necessary to support higher precision rowtime and proctime?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r343553792
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/GenerateUtils.scala
##
@@ -370,8 +370,15 @@ object GenerateUtils {
generateNonNullLiteral(literalType, literalValue.toString,
literalValue)
case TIMESTAMP_WITHOUT_TIME_ZONE =>
+// TODO: support Timestamp(3) now
+val fieldTerm = newName("timestamp")
val millis = literalValue.asInstanceOf[Long]
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r343553591
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/ExpressionReducer.scala
##
@@ -172,6 +170,15 @@ class ExpressionReducer(
}
reducedValues.add(maySkipNullLiteralReduce(rexBuilder, value,
unreduced))
reducedIdx += 1
+ case SqlTypeName.TIMESTAMP =>
+val reducedValue = reduced.getField(reducedIdx)
+val value = if (reducedValue != null) {
+ Long.box(reducedValue.asInstanceOf[SqlTimestamp].getMillisecond)
Review comment:
Just want break the original sql timestamp literal logic.
I think an individual commit would catch this, and let the sql timestamp
literal support precision.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r343553894
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/GenerateUtils.scala
##
@@ -438,10 +445,13 @@ object GenerateUtils {
def generateProctimeTimestamp(
ctx: CodeGeneratorContext,
contextTerm: String): GeneratedExpression = {
-val resultTerm = ctx.addReusableLocalVariable("long", "result")
+val resultType = new TimestampType(3)
+val resultTypeTerm = primitiveTypeTermForType(resultType)
+val resultTerm = ctx.addReusableLocalVariable(resultTypeTerm, "result")
val resultCode =
s"""
- |$resultTerm = $contextTerm.timerService().currentProcessingTime();
+ |$resultTerm = $SQL_TIMESTAMP_TERM.fromEpochMillis(
+ | $contextTerm.timerService().currentProcessingTime());
|""".stripMargin.trim
// the proctime has been materialized, so it's TIMESTAMP now, not
PROCTIME_INDICATOR
GeneratedExpression(resultTerm, NEVER_NULL, resultCode, new
TimestampType(3))
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r343549680
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/CodeGeneratorContext.scala
##
@@ -449,12 +450,14 @@ class CodeGeneratorContext(val tableConfig: TableConfig)
{
val timestamp = addReusableTimestamp()
// declaration
-reusableMemberStatements.add(s"private long $fieldTerm;")
+reusableMemberStatements.add(s"private $SQL_TIMESTAMP_TERM $fieldTerm;")
Review comment:
All `java.sql.Timestamp` and` java.time.LocalDateTime` in our engine should
be represented as `SqlTimestamp` now.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] docete commented on a change in pull request #10105: [Flink-14599][table-planner-blink] Support precision of TimestampType in blink planner
docete commented on a change in pull request #10105:
[Flink-14599][table-planner-blink] Support precision of TimestampType in blink
planner
URL: https://github.com/apache/flink/pull/10105#discussion_r343548788
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/CodeGenUtils.scala
##
@@ -97,6 +97,8 @@ object CodeGenUtils {
val STRING_UTIL: String = className[BinaryStringUtil]
+ val SQL_TIMESTAMP_TERM: String = className[SqlTimestamp]
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:
us...@infra.apache.org
With regards,
Apache Git Services
