hanyuzheng7 commented on code in PR #25897:
URL: https://github.com/apache/flink/pull/25897#discussion_r1948364110
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/DateTimeUtils.java:
##########
@@ -1290,6 +1353,24 @@ private static long floor(long a, long b) {
}
}
+ private static long floor(long a, int b) {
Review Comment:
I use a and b just follow other exist method, yes, I think I can rename a
and b and let them more meaning for.
q stand for quotient and r stand for remainder.
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/DateTimeUtils.java:
##########
@@ -1299,6 +1380,39 @@ private static long ceil(long a, long b) {
}
}
+ private static long ceil(long a, int b) {
+ float q = (float) b / NANOS_PER_MILLISECOND;
+ if (q > 0) {
+ return a + 1L;
+ } else {
+ return a;
+ }
+ }
+
+ private static int ceilForHighPrecision(int b) {
Review Comment:
Ok, I will rename it.
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/DateTimeUtils.java:
##########
@@ -1290,6 +1353,24 @@ private static long floor(long a, long b) {
}
}
+ private static long floor(long a, int b) {
+ long q = b / NANOS_PER_MILLISECOND;
+ if (q < 0) {
+ return a - q;
+ } else {
+ return a;
+ }
+ }
Review Comment:
Not cover any test, Yes, I think we need check if a is negative, if a < 0 we
need return a + q, otherwise return a, because we need handle the case when
utcTs < 0, because in this case, it stand for the time
before 1970.
The logic should be
```
return a > 0 ? a : a + q
```
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/DateTimeUtils.java:
##########
@@ -1299,6 +1380,39 @@ private static long ceil(long a, long b) {
}
}
+ private static long ceil(long a, int b) {
+ float q = (float) b / NANOS_PER_MILLISECOND;
+ if (q > 0) {
+ return a + 1L;
+ } else {
+ return a;
+ }
+ }
+
+ private static int ceilForHighPrecision(int b) {
+ long q = b / NANOS_PER_MICROSECOND;
Review Comment:
Actually except a and b, other parameters has its' meaning.
The reason I use a and b is other floor and ceil functions use a and b, so I
followed them. eg:
https://github.com/apache/flink/pull/25897/files#diff-e29751e95291efb2d7e399b60ee820322a29d24a716b7373a121a5d4d617aefbR1374
q stand for quotient and r stand for remainder.
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/BuiltInMethods.scala:
##########
@@ -386,15 +386,29 @@ object BuiltInMethods {
"timestampFloor",
classOf[TimeUnitRange],
classOf[Long],
+ classOf[Int],
classOf[TimeZone])
+ val TIMESTAMP_FLOOR_TIME_ZONE_FOR_HIGH_PRECISiON = Types.lookupMethod(
Review Comment:
Will update the name.
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/DateTimeUtils.java:
##########
@@ -1299,6 +1380,39 @@ private static long ceil(long a, long b) {
}
}
+ private static long ceil(long a, int b) {
+ float q = (float) b / NANOS_PER_MILLISECOND;
Review Comment:
The logic should like this
```
return a > 0 ? a + 1L : a
```
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/TimeFunctionsITCase.java:
##########
@@ -50,7 +50,7 @@
class TimeFunctionsITCase extends BuiltInFunctionTestBase {
private static final DateTimeFormatter TIMESTAMP_FORMATTER =
- DateTimeFormatter.ofPattern("yyyy-MM-dd' 'HH:mm:ss.SSS");
+ DateTimeFormatter.ofPattern("yyyy-MM-dd' 'HH:mm:ss.SSSSSSSSS");
Review Comment:
No, because we test nano second and microsecond at this time, we have to
increase the precision, otherwise we cannot test whether last 6 digits is
correct or not.
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/DateTimeUtils.java:
##########
@@ -1290,6 +1353,24 @@ private static long floor(long a, long b) {
}
}
+ private static long floor(long a, int b) {
+ long q = b / NANOS_PER_MILLISECOND;
+ if (q < 0) {
+ return a - q;
+ } else {
+ return a;
+ }
+ }
+
+ private static int floorForHighPrecision(int b) {
+ long q = b / NANOS_PER_MICROSECOND;
Review Comment:
We don't need check if q < 0 or not, we need check if utcTs < 0 here.
So the logic should like this
```
return (a < 0 ? q -1 : q) * NANOS_PER_MICROSECOND;
```
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/DateTimeUtils.java:
##########
@@ -1275,7 +1314,31 @@ public static long timestampCeil(TimeUnitRange range,
long ts, TimeZone tz) {
int days = (int) (utcTs / MILLIS_PER_DAY + EPOCH_JULIAN);
return julianDateFloor(range, days, false) * MILLIS_PER_DAY -
offset;
default:
- // for MINUTE and SECONDS etc...,
+ // it is more effective to use arithmetic Method
+ throw new AssertionError(range);
+ }
+ }
+
+ public static int timestampCeilForHighPrecision(TimeUnitRange range, int
ns) {
+ switch (range) {
+ case NANOSECOND:
+ return ns;
+ case MICROSECOND:
+ return ceilForHighPrecision(ns);
+ case MILLISECOND:
Review Comment:
if we can handle below specific case, we can optimize the logic for these
two methods, but we cannot remove them, because these two method return
nanosecond. We cannot use one method, because of the structure for
FloorCeilCallGen.scala, we have to deliver two methods together.
The specific case is here:
`TimestampData` has a restriction
```
private TimestampData(long millisecond, int nanoOfMillisecond) {
Preconditions.checkArgument(nanoOfMillisecond >= 0 &&
nanoOfMillisecond <= 999_999);
this.millisecond = millisecond;
this.nanoOfMillisecond = nanoOfMillisecond;
}
```
The `nanoOfMillisecond <= 999_999`, if we change it to `nanoOfMillisecond <=
1_000_000`, when we meet `999_999 + 1` this situation, it will not throw
exception. So we don't have to need a logic to check this specific situation.
Now when this situation happen, `999_999 + 1 = 1_000_000`
ceilForHighPrecision will return 0, and ceilForHighPrecision will add 1 to
handle this specific case.
```
private static int ceilForHighPrecision(int b) {
long q = b / NANOS_PER_MICROSECOND;
long r = b % NANOS_PER_MICROSECOND;
if (q > 0 && r > 0) {
if (q + 1 == NANOS_PER_MICROSECOND) {
return 0;
}
return (int) ((q + 1) * NANOS_PER_MICROSECOND);
} else {
return (int) (q * NANOS_PER_MICROSECOND);
}
}
```
```
private static long ceilForHighPrecision(long a, int b) {
long q = b / NANOS_PER_MICROSECOND;
long r = b % NANOS_PER_MICROSECOND;
if (q > 0 && r > 0) {
if (q + 1 == NANOS_PER_MICROSECOND) {
return a + 1L;
}
}
return a;
}
```
--
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]
