amogh-jahagirdar commented on code in PR #9008:
URL: https://github.com/apache/iceberg/pull/9008#discussion_r1664436827
##########
api/src/main/java/org/apache/iceberg/transforms/Timestamps.java:
##########
@@ -31,54 +33,160 @@
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);
+ }
+ }
+
+ static Timestamps get(Types.TimestampNanoType 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(Types.TimestampType type, ChronoUnit resultTypeUnit) {
+ 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;
+ default:
+ throw new IllegalArgumentException(
+ "Unsupported source/result type units: " + type + " -> " +
resultTypeUnit);
+ }
+ }
+
+ static Timestamps get(Types.TimestampNanoType type, ChronoUnit
resultTypeUnit) {
+ 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;
+ default:
+ throw new IllegalArgumentException(
+ "Unsupported source/result type units: " + type + " -> " +
resultTypeUnit);
+ }
+ }
+
+ enum ResultTypeUnit {
+ YEARS(ChronoUnit.YEARS),
+ MONTHS(ChronoUnit.MONTHS),
+ DAYS(ChronoUnit.DAYS),
+ HOURS(ChronoUnit.HOURS),
+ MICROS(ChronoUnit.MICROS),
+ NANOS(ChronoUnit.NANOS),
+ ;
Review Comment:
Can you remove the unnecessary semicolon?
##########
api/src/test/java/org/apache/iceberg/TestPartitionPaths.java:
##########
@@ -54,6 +54,25 @@ public void testPartitionPath() {
.isEqualTo("ts_hour=2017-12-01-10/id_bucket=" + idBucket);
}
+ @Test
+ public void testPartitionPathWithNanoseconds() {
+ PartitionSpec spec =
PartitionSpec.builderFor(SCHEMA).hour("ts").bucket("id", 10).build();
+
+ Transform<Long, Integer> hour = Transforms.hour();
+ Transform<Integer, Integer> bucket = Transforms.bucket(10);
+
+ Literal<Long> ts =
+
Literal.of("2017-12-01T10:12:55.038194789").to(Types.TimestampNanoType.withoutZone());
+ Object tsHour =
hour.bind(Types.TimestampNanoType.withoutZone()).apply(ts.value());
Review Comment:
Can you add another case where it's timestamptz_ns?
##########
api/src/test/java/org/apache/iceberg/transforms/TestBucketing.java:
##########
@@ -165,6 +212,57 @@ public void testLong() {
.isEqualTo(hashBytes(buffer.array()));
}
+ @Test
+ public void testTimestampNanoPromotion() {
Review Comment:
Can we break this test down further? Feel like it's testing a lot in a
single method. At least separation between timestamp with and without timezone
seems feasible.
##########
api/src/main/java/org/apache/iceberg/transforms/Timestamps.java:
##########
@@ -31,54 +33,160 @@
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);
+ }
+ }
+
+ static Timestamps get(Types.TimestampNanoType 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(Types.TimestampType type, ChronoUnit resultTypeUnit) {
+ 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;
+ default:
+ throw new IllegalArgumentException(
+ "Unsupported source/result type units: " + type + " -> " +
resultTypeUnit);
+ }
+ }
+
+ static Timestamps get(Types.TimestampNanoType type, ChronoUnit
resultTypeUnit) {
+ 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;
+ default:
+ throw new IllegalArgumentException(
+ "Unsupported source/result type units: " + type + " -> " +
resultTypeUnit);
+ }
+ }
+
+ enum ResultTypeUnit {
+ YEARS(ChronoUnit.YEARS),
+ MONTHS(ChronoUnit.MONTHS),
+ DAYS(ChronoUnit.DAYS),
+ HOURS(ChronoUnit.HOURS),
+ MICROS(ChronoUnit.MICROS),
+ NANOS(ChronoUnit.NANOS),
+ ;
+
+ private final ChronoUnit unit;
+
+ ResultTypeUnit(final ChronoUnit unit) {
+ this.unit = unit;
+ }
+
+ Duration duration() {
+ return unit.getDuration();
+ }
+ }
@Immutable
static class Apply implements SerializableFunction<Long, Integer> {
- private final ChronoUnit granularity;
+ private final ChronoUnit sourceTypeUnit;
+ private final ResultTypeUnit resultTypeUnit;
- Apply(ChronoUnit granularity) {
- this.granularity = granularity;
+ Apply(ChronoUnit sourceTypeUnit, ResultTypeUnit resultTypeUnit) {
+ this.sourceTypeUnit = sourceTypeUnit;
+ this.resultTypeUnit = resultTypeUnit;
}
@Override
- public Integer apply(Long timestampMicros) {
- if (timestampMicros == null) {
+ public Integer apply(Long timestampUnits) {
Review Comment:
Feels a bit odd to call this variable `timestampUnits`. The actual units is
passed to the class as part of `sourceTypeUnit`. I'd just call it `timetamp`.
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java:
##########
@@ -594,6 +605,12 @@ private static String sanitizeString(CharSequence value,
long now, int today) {
if (DATE.matcher(value).matches()) {
Literal<Integer> date = Literal.of(value).to(Types.DateType.get());
return sanitizeDate(date.value(), today);
+ } else if (TIMESTAMPNS.matcher(value).matches()) {
+ Literal<Long> ts =
Literal.of(value).to(Types.TimestampNanoType.withoutZone());
+ return sanitizeTimestamp(Math.floorDiv(ts.value(), 1000), now);
Review Comment:
Any reason not to use the DateTimeUtil.nanosToMicros API that was added
(instead of just doing the div here)?
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -259,6 +261,60 @@ public int hashCode() {
}
}
+ public static class TimestampNanoType extends PrimitiveType {
+ private static final TimestampNanoType INSTANCE_WITH_ZONE = new
TimestampNanoType(true);
+ private static final TimestampNanoType INSTANCE_WITHOUT_ZONE = new
TimestampNanoType(false);
+
+ public static TimestampNanoType withZone() {
+ return INSTANCE_WITH_ZONE;
+ }
+
+ public static TimestampNanoType withoutZone() {
+ return INSTANCE_WITHOUT_ZONE;
+ }
+
+ private final boolean adjustToUTC;
+
+ private TimestampNanoType(boolean adjustToUTC) {
+ this.adjustToUTC = adjustToUTC;
+ }
+
+ public boolean shouldAdjustToUTC() {
+ return adjustToUTC;
+ }
+
+ @Override
+ public TypeID typeId() {
+ return TypeID.TIMESTAMP_NANO;
+ }
+
+ @Override
+ public String toString() {
+ if (shouldAdjustToUTC()) {
+ return "timestamptz_ns";
+ } else {
+ return "timestamp_ns";
+ }
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ } else if (!(o instanceof TimestampNanoType)) {
+ return false;
+ }
+
+ TimestampNanoType that = (TimestampNanoType) o;
+ return adjustToUTC == that.adjustToUTC;
Review Comment:
super nit, this may be a case just to call the parameter `other` and inline:
```
return adjustToUTC == ((TimestampNanoType) other).adjustToUTC
```
--
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
