nastra commented on code in PR #9008:
URL: https://github.com/apache/iceberg/pull/9008#discussion_r1627706644
##########
api/src/main/java/org/apache/iceberg/util/DateTimeUtil.java:
##########
@@ -75,6 +77,14 @@ public static long microsToMillis(long micros) {
return Math.floorDiv(micros, MICROS_PER_MILLIS);
}
+ public static long nanosToMicros(long nanos) {
Review Comment:
it's a good idea to add new tests for every single method that is being
added here. These tests should go into `TestDateTimeUtil`
##########
api/src/main/java/org/apache/iceberg/transforms/Days.java:
##########
@@ -55,7 +57,16 @@ public boolean satisfiesOrderOf(Transform<?, ?> other) {
}
if (other instanceof Timestamps) {
- return Timestamps.DAY.satisfiesOrderOf(other);
+ Timestamps.ResultTypeUnit otherResultTypeUnit = ((Timestamps)
other).resultTypeUnit();
+ switch (otherResultTypeUnit) {
+ case MICROS:
+ return Timestamps.DAY_FROM_MICROS.satisfiesOrderOf(other);
+ case NANOS:
+ return Timestamps.DAY_FROM_NANOS.satisfiesOrderOf(other);
+ default:
+ throw new UnsupportedOperationException(
+ "Unsupported timestamp unit: " + otherResultTypeUnit);
Review Comment:
I think it would be good to add a test that exercises this path here
##########
api/src/main/java/org/apache/iceberg/transforms/Transforms.java:
##########
@@ -188,9 +198,15 @@ public static <T> Transform<T, Integer> day(Type type) {
@Deprecated
@SuppressWarnings("unchecked")
public static <T> Transform<T, Integer> hour(Type type) {
- Preconditions.checkArgument(
- type.typeId() == Type.TypeID.TIMESTAMP, "Cannot partition type %s by
hour", type);
- return (Transform<T, Integer>) Timestamps.HOUR;
+ switch (type.typeId()) {
+ case TIMESTAMP:
+ return (Transform<T, Integer>) Timestamps.HOUR_FROM_MICROS;
+ case TIMESTAMP_NANO:
+ return (Transform<T, Integer>) Timestamps.HOUR_FROM_NANOS;
+ default:
+ throw new IllegalArgumentException(
+ Strings.lenientFormat("Cannot partition type %s by hour", type));
Review Comment:
why not use `String.format` here?
##########
api/src/main/java/org/apache/iceberg/transforms/SortOrderVisitor.java:
##########
@@ -85,21 +85,18 @@ static <R> List<R> visit(SortOrder sortOrder,
SortOrderVisitor<R> visitor) {
visitor.truncate(
sourceName, field.sourceId(), width, field.direction(),
field.nullOrder()));
} else if (transform == Dates.YEAR
- || transform == Timestamps.YEAR
+ || transform == Timestamps.YEAR_FROM_MICROS
+ || transform == Timestamps.YEAR_FROM_NANOS
|| transform instanceof Years) {
results.add(
visitor.year(sourceName, field.sourceId(), field.direction(),
field.nullOrder()));
- } else if (transform == Dates.MONTH
- || transform == Timestamps.MONTH
- || transform instanceof Months) {
+ } else if ("month".equalsIgnoreCase(transform.toString())) {
Review Comment:
is there a particular reason to switch to string comparison here and in
other places?
##########
api/src/main/java/org/apache/iceberg/transforms/Hours.java:
##########
@@ -33,16 +33,19 @@ static <T> Hours<T> get() {
@Override
@SuppressWarnings("unchecked")
protected Transform<T, Integer> toEnum(Type type) {
- if (type.typeId() == Type.TypeID.TIMESTAMP) {
- return (Transform<T, Integer>) Timestamps.HOUR;
+ switch (type.typeId()) {
+ case TIMESTAMP:
+ return (Transform<T, Integer>) Timestamps.HOUR_FROM_MICROS;
+ case TIMESTAMP_NANO:
+ return (Transform<T, Integer>) Timestamps.HOUR_FROM_NANOS;
+ default:
+ throw new IllegalArgumentException("Unsupported type: " + type);
Review Comment:
nit: I think it would be good to have a unit test that exercises this path
##########
api/src/main/java/org/apache/iceberg/transforms/Years.java:
##########
@@ -55,7 +57,16 @@ public boolean satisfiesOrderOf(Transform<?, ?> other) {
}
if (other instanceof Timestamps) {
- return Timestamps.YEAR.satisfiesOrderOf(other);
+ Timestamps.ResultTypeUnit otherResultTypeUnit = ((Timestamps)
other).resultTypeUnit();
+ switch (otherResultTypeUnit) {
+ case MICROS:
+ return Timestamps.YEAR_FROM_MICROS.satisfiesOrderOf(other);
+ case NANOS:
+ return Timestamps.YEAR_FROM_NANOS.satisfiesOrderOf(other);
+ default:
+ throw new UnsupportedOperationException(
+ "Unsupported timestamp unit: " + otherResultTypeUnit);
Review Comment:
should have a test
##########
api/src/main/java/org/apache/iceberg/transforms/Timestamps.java:
##########
@@ -31,54 +33,166 @@
import org.apache.iceberg.util.DateTimeUtil;
import org.apache.iceberg.util.SerializableFunction;
-enum Timestamps implements Transform<Long, Integer> {
- YEAR(ChronoUnit.YEARS, "year"),
- MONTH(ChronoUnit.MONTHS, "month"),
- DAY(ChronoUnit.DAYS, "day"),
- HOUR(ChronoUnit.HOURS, "hour");
+class Timestamps implements Transform<Long, Integer> {
+
+ static final Timestamps YEAR_FROM_MICROS =
+ new Timestamps(ChronoUnit.MICROS, ResultTypeUnit.YEARS, "year");
+ static final Timestamps MONTH_FROM_MICROS =
+ new Timestamps(ChronoUnit.MICROS, ResultTypeUnit.MONTHS, "month");
+ static final Timestamps DAY_FROM_MICROS =
+ new Timestamps(ChronoUnit.MICROS, ResultTypeUnit.DAYS, "day");
+ static final Timestamps HOUR_FROM_MICROS =
+ new Timestamps(ChronoUnit.MICROS, ResultTypeUnit.HOURS, "hour");
+ static final Timestamps YEAR_FROM_NANOS =
+ new Timestamps(ChronoUnit.NANOS, ResultTypeUnit.YEARS, "year");
+ static final Timestamps MONTH_FROM_NANOS =
+ new Timestamps(ChronoUnit.NANOS, ResultTypeUnit.MONTHS, "month");
+ static final Timestamps DAY_FROM_NANOS =
+ new Timestamps(ChronoUnit.NANOS, ResultTypeUnit.DAYS, "day");
+ static final Timestamps HOUR_FROM_NANOS =
+ new Timestamps(ChronoUnit.NANOS, ResultTypeUnit.HOURS, "hour");
+
+ static Timestamps get(Types.TimestampType type, String resultTypeUnit) {
+ switch (resultTypeUnit.toLowerCase(Locale.ENGLISH)) {
+ case "year":
+ return get(type, ChronoUnit.YEARS);
+ case "month":
+ return get(type, ChronoUnit.MONTHS);
+ case "day":
+ return get(type, ChronoUnit.DAYS);
+ case "hour":
+ return get(type, ChronoUnit.HOURS);
+ default:
+ throw new IllegalArgumentException(
+ "Unsupported source/result type units: " + type + "->" +
resultTypeUnit);
Review Comment:
it would be good to have tests for all of these error conditions
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
