Transforming timestamp to date should produce date

[rdblue]

The output type of transforming a timestamp to a date Transforms.day(Types.TimestampType.withZone() is an integer, but should be a date. That way, it will be formatted as a date in metadata tables.

When this is updated, we need to validate backward compatibility with existing tables.

[electrum]

Should day also not support the date type, since that would be an identity transform?

[rdblue]

You're probably right, although there shouldn't be a difference between the two in practice, once strict projection for dates is added.

[timmylicheng]

What are suggested ways to validate backward compatibility?

[waterlx]

Hi @rdblue please correct me if I am wrong:
I am not sure if Transforms.day() is the function to transfer timestamps to date, it is to produce Dates.DAY or Timestamp.DAY (either is Transform) from Types.DateType or Types.TimestampType. By this issue, are you proposing a new method in Transforms which returns Transform<T, java.util.Date> from Types.DateType or Types.TimestampType?

[electrum]

Use java.time.LocalDate. The older java.util.Date actually represents an instant.

[rdblue]

@waterlx, the day transform reports that its return type is IntegerType.get(), but it should be DateType.get(). Both are stored as integers but the transform actually produces the internal value that represents a date: the number of days from the unix epoch.

Changing the return type to date should be safe and would cause engines that support the metadata tables to format the values correctly as dates instead of as integers.

@timmylicheng, the compatibility checks just need to verify that older tables that are partitioned by day can still be read. It's just a matter of writing a table with the current master, making this change and reading with the new library. We don't expect any exceptions while reading or filtering after the update.

[manishmalhotrawork]

@rdblue if Im not missing, in this case
org.apache.iceberg.transforms.Timestamps will implement Transform<Long, Date> and org.apache.iceberg.transforms.Dates will implement Transform<Integer, Date>.
and internally these classes will use Date object for various transformation like apply , toHumanString.

@waterlx are you working on this issue ?

[rdblue]

@manishmalhotrawork, only the day transform should return a date. Years, months, and hours should continue returning ordinals like before.

[waterlx]

@rdblue, regarding

@manishmalhotrawork, only the day transform should return a date. Years, months, and hours should continue returning ordinals like before.

so you mean that we are supposed to keep org.apache.iceberg.transforms.Timestamps as it is (as Transform<Long, Integer>) and keep org.apache.iceberg.transforms.Dates as it is (as Transform<Integer, Integer>), so as not to change the return of Transforms.year(), month() and hour()? Do you propose to only change the output of Transforms.day() to Transform<T, Date>? Do I get it right?

@manishmalhotrawork yes, I am working on this issue. But please feel free to go ahead to share your thoughts or upload patches, I am new to iceberg

[rdblue]

Sorry for the confusion. I'm talking about the Iceberg date type, Types.DateType.get(). The transform implementation is still going to return a Java Integer because that's how dates are represented in Iceberg metadata: the number of days from the Unix epoch.

This change would primarily just update getResultType and return DateType.get() when the granularity is ChronoUnit.DAYS. That will cause the partition tuple's field type to be a date, which should also cause the metadata table to display formatted dates instead of the day ordinal in Spark.

[manishmalhotrawork]

@rdblue

This change would primarily just update getResultType and return DateType.get() when the granularity is ChronoUnit.DAYS.

this is bit more clear now. thanks !

so this will change ( if granularity is Days)

1. Dates.getResultType

public Type getResultType(Type sourceType) {
    return Types.IntegerType.get();
  }

and what about Timestamps.getResultType , does it also needs to return Types.DateType.get() , as that can also be needed to print datetime in metadata table. ?

[rdblue]

Yes, I think both should return DateType

[waterlx]

Thanks for the clarification, Ryan! I am drafting the patch.

[manishmalhotrawork]

@rdblue thanks for the answer.
Just saw messages, before that I also drafted the patch, waiting for internal approval.
@waterlx I can push, and then you can also review? Or otherwise please let me know.