raminqaf commented on code in PR #29080:
URL: https://github.com/apache/flink/pull/29080#discussion_r3924929701
##########
docs/content.zh/docs/sql/reference/data-types.md:
##########
Review Comment:
Are we planing to add more docs on the assumptions we take for the TIME
precision cast?
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/casting/CastRuleProviderTest.java:
##########
@@ -127,11 +127,10 @@ void testResolveVariantToPrimitive() {
assertThat(CastRuleProvider.exists(VARIANT,
DATE().getLogicalType())).isTrue();
assertThat(CastRuleProvider.exists(VARIANT,
TIMESTAMP().getLogicalType())).isTrue();
assertThat(CastRuleProvider.exists(VARIANT,
TIMESTAMP_LTZ().getLogicalType())).isTrue();
+ assertThat(CastRuleProvider.exists(VARIANT,
TIME().getLogicalType())).isTrue();
assertThat(CastRuleProvider.exists(VARIANT,
BYTES().getLogicalType())).isTrue();
assertThat(CastRuleProvider.canFail(VARIANT, INT)).isTrue();
- // TIME has no variant counterpart and is not castable
- assertThat(CastRuleProvider.exists(VARIANT,
TIME().getLogicalType())).isFalse();
Review Comment:
Maybe good to add a internal type like INTERVAL as a not castable case
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/VariantCastUtils.java:
##########
@@ -58,9 +58,18 @@ public final class VariantCastUtils {
*/
private static final double LONG_MAGNITUDE_LIMIT = -(double)
Long.MIN_VALUE;
- /** A variant stores a timestamp with microsecond precision. */
+ /** A microsecond timestamp variant renders with six fractional-second
digits. */
private static final int TIMESTAMP_PRECISION = 6;
+ /** A nanosecond timestamp variant renders with nine fractional-second
digits. */
Review Comment:
I think this javaDoc is not really needed
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/casting/CastRulesTest.java:
##########
Review Comment:
Let's add a test with precision to the default TIMESTAMP precision
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/VariantCastUtils.java:
##########
@@ -194,12 +205,27 @@ public static TimestampData toTimestamp(Variant variant,
int precision) {
/** Reads a timestamp with local time zone variant. See {@link
#toTimestamp(Variant, int)}. */
public static TimestampData toTimestampLtz(Variant variant, int precision)
{
- if (variant.getType() != Variant.Type.TIMESTAMP_LTZ) {
+ final Variant.Type type = variant.getType();
+ if (type != Variant.Type.TIMESTAMP_LTZ && type !=
Variant.Type.TIMESTAMP_LTZ_NS) {
Review Comment:
How about flipping the if statement?
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/casting/CastRulesTest.java:
##########
@@ -1783,6 +1801,25 @@ Stream<CastTestSpecBuilder> testCases() {
Variant.newBuilder().of(LocalDate.of(2020, 1,
1)),
(int) LocalDate.of(2020, 1, 1).toEpochDay())
.fail(VARIANT(), Variant.newBuilder().of(1),
TableRuntimeException.class),
+ // A variant keeps microseconds for TIME, so fractional
seconds beyond the target
+ // precision are truncated, matching a regular cast into a
narrower TIME.
+ CastTestSpecBuilder.testCastTo(TIME(3))
+ .fromCase(
+ VARIANT(),
+ Variant.newBuilder().of(LocalTime.of(12, 0, 0,
123_000_000)),
Review Comment:
This is up to you but I kinda like this API a bit more:
```suggestion
Variant.newBuilder().of(LocalTime.of(12, 0,
0).plus(Duration.ofMillis(123)),
```
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/VariantCastUtils.java:
##########
@@ -58,9 +58,18 @@ public final class VariantCastUtils {
*/
private static final double LONG_MAGNITUDE_LIMIT = -(double)
Long.MIN_VALUE;
- /** A variant stores a timestamp with microsecond precision. */
+ /** A microsecond timestamp variant renders with six fractional-second
digits. */
private static final int TIMESTAMP_PRECISION = 6;
+ /** A nanosecond timestamp variant renders with nine fractional-second
digits. */
+ private static final int TIMESTAMP_NANOS_PRECISION = 9;
+
+ /**
+ * A time variant keeps microseconds, but the runtime TIME representation
is millisecond-of-day,
+ * so it renders with three fractional-second digits, the same as a
regular TIME to string cast.
+ */
Review Comment:
This can be mentioned the data-type docs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
