Fokko commented on code in PR #9008:
URL: https://github.com/apache/iceberg/pull/9008#discussion_r1520360025
##########
api/src/main/java/org/apache/iceberg/transforms/Days.java:
##########
@@ -55,14 +56,15 @@ public boolean satisfiesOrderOf(Transform<?, ?> other) {
}
if (other instanceof Timestamps) {
- return Timestamps.DAY.satisfiesOrderOf(other);
+ ChronoUnit otherResultTypeUnit = ((Timestamps)
other).getResultTypeUnit();
+ return otherResultTypeUnit == ChronoUnit.DAYS
+ || otherResultTypeUnit == ChronoUnit.MONTHS
+ || otherResultTypeUnit == ChronoUnit.YEARS;
Review Comment:
I'd prefer to keep the logic of this check inside of the `satisfiesOrderOf`
function:
```suggestion
ChronoUnit otherTimestamp = ((Timestamps) other).getResultTypeUnit();
if (otherTimestamp == ChronoUnit.MICROS) {
return Timestamps.DAY_FROM_MICROS.satisfiesOrderOf(other);
} else if (otherTimestamp == ChronoUnit.NANOS) {
return Timestamps.DAY_FROM_NANOS.satisfiesOrderOf(other);
} else {
throw new UnsupportedOperationException("Unsupported timestamp unit:
" + otherTimestamp);
}
```
Same goes for the other transforms.
##########
api/src/main/java/org/apache/iceberg/transforms/TransformUtil.java:
##########
@@ -54,12 +55,26 @@ static String humanTime(Long microsFromMidnight) {
return LocalTime.ofNanoOfDay(microsFromMidnight * 1000).toString();
}
- static String humanTimestampWithZone(Long timestampMicros) {
- return ChronoUnit.MICROS.addTo(EPOCH, timestampMicros).toString();
- }
-
- static String humanTimestampWithoutZone(Long timestampMicros) {
- return ChronoUnit.MICROS.addTo(EPOCH,
timestampMicros).toLocalDateTime().toString();
+ public static String humanTimestamp(Types.TimestampType tsType, Long value) {
+ if (tsType.shouldAdjustToUTC()) {
+ switch (tsType.unit()) {
+ case MICROS:
+ return ChronoUnit.MICROS.addTo(EPOCH, value).toString();
+ case NANOS:
+ return ChronoUnit.NANOS.addTo(EPOCH, value).toString();
+ default:
+ throw new UnsupportedOperationException("Unsupported timestamp unit:
" + tsType.unit());
+ }
+ } else {
+ switch (tsType.unit()) {
+ case MICROS:
+ return ChronoUnit.MICROS.addTo(EPOCH,
value).toLocalDateTime().toString();
+ case NANOS:
+ return ChronoUnit.NANOS.addTo(EPOCH,
value).toLocalDateTime().toString();
+ default:
+ throw new UnsupportedOperationException("Unsupported timestamp unit:
" + tsType.unit());
Review Comment:
Same here, we could just throw it once at the end of the method.
##########
api/src/main/java/org/apache/iceberg/transforms/Transforms.java:
##########
@@ -86,8 +87,9 @@ private Transforms() {}
try {
if (type.typeId() == Type.TypeID.TIMESTAMP) {
- return Timestamps.valueOf(transform.toUpperCase(Locale.ENGLISH));
- } else if (type.typeId() == Type.TypeID.DATE) {
+ return Timestamps.get((TimestampType) type, transform);
+ }
+ if (type.typeId() == Type.TypeID.DATE) {
Review Comment:
```suggestion
} else if (type.typeId() == Type.TypeID.DATE) {
```
##########
api/src/main/java/org/apache/iceberg/transforms/Timestamps.java:
##########
@@ -28,57 +29,131 @@
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.types.Type;
import org.apache.iceberg.types.Types;
+import org.apache.iceberg.types.Types.TimestampType;
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,
ChronoUnit.YEARS);
+ static final Timestamps MONTH_FROM_MICROS = new
Timestamps(ChronoUnit.MICROS, ChronoUnit.MONTHS);
+ static final Timestamps DAY_FROM_MICROS = new Timestamps(ChronoUnit.MICROS,
ChronoUnit.DAYS);
+ static final Timestamps HOUR_FROM_MICROS = new Timestamps(ChronoUnit.MICROS,
ChronoUnit.HOURS);
+ static final Timestamps YEAR_FROM_NANOS = new Timestamps(ChronoUnit.NANOS,
ChronoUnit.YEARS);
+ static final Timestamps MONTH_FROM_NANOS = new Timestamps(ChronoUnit.NANOS,
ChronoUnit.MONTHS);
+ static final Timestamps DAY_FROM_NANOS = new Timestamps(ChronoUnit.NANOS,
ChronoUnit.DAYS);
+ static final Timestamps HOUR_FROM_NANOS = new Timestamps(ChronoUnit.NANOS,
ChronoUnit.HOURS);
+
+ static Timestamps get(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);
+ }
+ }
+
+ static Timestamps get(TimestampType type, ChronoUnit resultTypeUnit) {
+ switch (type.unit()) {
+ case MICROS:
+ switch (resultTypeUnit) {
+ case YEARS:
+ return YEAR_FROM_MICROS;
+ case MONTHS:
+ return MONTH_FROM_MICROS;
+ case DAYS:
+ return DAY_FROM_MICROS;
+ case HOURS:
+ return HOUR_FROM_MICROS;
+ }
+ break;
+ case NANOS:
+ switch (resultTypeUnit) {
+ case YEARS:
+ return YEAR_FROM_NANOS;
+ case MONTHS:
+ return MONTH_FROM_NANOS;
+ case DAYS:
+ return DAY_FROM_NANOS;
+ case HOURS:
+ return HOUR_FROM_NANOS;
+ }
+ break;
+ default:
+ throw new UnsupportedOperationException("Unsupported timestamp unit: "
+ type.unit());
+ }
+ throw new IllegalArgumentException(
+ "Unsupported source/result type units: " + type + "->" +
resultTypeUnit);
+ }
@Immutable
static class Apply implements SerializableFunction<Long, Integer> {
- private final ChronoUnit granularity;
+ private final ChronoUnit sourceTypeUnit;
+ private final ChronoUnit resultTypeUnit;
- Apply(ChronoUnit granularity) {
- this.granularity = granularity;
+ Apply(ChronoUnit sourceTypeUnit, ChronoUnit resultTypeUnit) {
+ this.sourceTypeUnit = sourceTypeUnit;
+ this.resultTypeUnit = resultTypeUnit;
}
@Override
- public Integer apply(Long timestampMicros) {
- if (timestampMicros == null) {
+ public Integer apply(Long timestampUnits) {
+ if (timestampUnits == null) {
return null;
}
- switch (granularity) {
- case YEARS:
- return DateTimeUtil.microsToYears(timestampMicros);
- case MONTHS:
- return DateTimeUtil.microsToMonths(timestampMicros);
- case DAYS:
- return DateTimeUtil.microsToDays(timestampMicros);
- case HOURS:
- return DateTimeUtil.microsToHours(timestampMicros);
+ switch (sourceTypeUnit) {
+ case MICROS:
+ switch (resultTypeUnit) {
+ case YEARS:
+ return DateTimeUtil.microsToYears(timestampUnits);
+ case MONTHS:
+ return DateTimeUtil.microsToMonths(timestampUnits);
+ case DAYS:
+ return DateTimeUtil.microsToDays(timestampUnits);
+ case HOURS:
+ return DateTimeUtil.microsToHours(timestampUnits);
+ default:
+ throw new UnsupportedOperationException(
+ "Unsupported result type unit: " + resultTypeUnit);
+ }
+ case NANOS:
+ switch (resultTypeUnit) {
+ case YEARS:
+ return DateTimeUtil.nanosToYears(timestampUnits);
+ case MONTHS:
+ return DateTimeUtil.nanosToMonths(timestampUnits);
+ case DAYS:
+ return DateTimeUtil.nanosToDays(timestampUnits);
+ case HOURS:
+ return DateTimeUtil.nanosToHours(timestampUnits);
+ default:
+ throw new UnsupportedOperationException(
+ "Unsupported result type unit: " + resultTypeUnit);
+ }
default:
- throw new UnsupportedOperationException("Unsupported time unit: " +
granularity);
+ throw new UnsupportedOperationException(
+ "Unsupported source type unit: " + sourceTypeUnit);
Review Comment:
Style nit: Instead of throwing the exception three times, it is possible to
just do it once after the outer case statement.
--
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
