AFAIK, Presto uses 4 bytes for DateType as well as for IntegerType for its internal representation, but java signature is Long, so from an API point of view, it is long. When Presto constructs iceberg Predicates using, for example, equal(String name, T value), the actual parameter is of type Long, so iceberg creates LongLiteral that later is converted to Date. It is possible to workaround that, but IMO, it is natural to expect that if LongLiteral is convertible to IntegerLiteral and IntegerLiteral is convertible to DateLiteral, LongLiteral should be convertible to DateLiteral directly (assuming Long->Integer works) and avoid workarounds/special handling in Presto or other systems that utilize Long for Date types.
Thank you, Vlad On 2020/02/19 21:07:25, Ryan Blue <rb...@netflix.com.INVALID> wrote: > I'm not quite following. What is the internal representation that Presto > uses for dates? > > On Wed, Feb 19, 2020 at 12:58 PM Vlad Rozov <vro...@apache.org> wrote: > > > While it is possible to convert to IntLiteral or even probably to > > DateLiteral, presto mostly delegates to iceberg to do the proper > > conversion from LongLiteral, AFAIK (see > > https://github.com/prestosql/presto/blob/de97d1572d7da5570177627bd42fbb8b7fdd417e/presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/ExpressionConverter.java#L167 > > ) > > > > Thank you, > > > > Vlad > > > > On 2020/02/19 20:10:14, Ryan Blue <rb...@netflix.com.INVALID> wrote: > > > Can you describe the use case a bit more? What prevents you from using an > > > IntLiteral instead? > > > > > > On Wed, Feb 19, 2020 at 11:26 AM Vlad Rozov <vro...@apache.org> wrote: > > > > > > > Hi Ryan, > > > > > > > > Thank you for the detailed explanation. Yes, there is a use case in > > Presto > > > > for LongLiteral to DateLiteral conversion as it uses long and allows > > it to > > > > be converted/cast to Date. > > > > > > > > Thank you, > > > > > > > > Vlad > > > > > > > > On 2020/02/19 18:54:35, Ryan Blue <rb...@netflix.com.INVALID> wrote: > > > > > Originally, we didn't allow int to date or long to timestamp, but we > > > > added > > > > > those to support expression conversion from Spark. It's much easier > > to > > > > > allow the LongLiteral created from a Spark timestamp expression > > directly > > > > to > > > > > a TimestampLiteral than to convert to an equivalent timestamp string > > > > > because the internal representation in Spark and Iceberg are the > > same. > > > > The > > > > > conversion from long to date was never really needed by this path, > > so we > > > > > probably didn't add it. Iceberg is fairly strict with allowed > > conversions > > > > > to date and timestamp because the validation we can apply is still > > very > > > > > permissive. > > > > > > > > > > I think it's fine to add long to date if there is a need, but > > otherwise > > > > I'd > > > > > leave it as it is. Do you have a use case for this? > > > > > > > > > > On Tue, Feb 18, 2020 at 3:19 PM Vlad Rozov <vro...@apache.org> > > wrote: > > > > > > > > > > > Hi, > > > > > > > > > > > > What is the reason iceberg does not allow LongLiteral to > > DateLiteral > > > > > > conversion while allowing LongLiteral to IntegerLiteral and > > > > IntegerLiteral > > > > > > to DateLiteral? Should not direct conversion from LongLiteral to > > > > > > DateLiteral be allowed when LongLiteral represents values in a > > proper > > > > range? > > > > > > > > > > > > Thank you, > > > > > > > > > > > > Vlad > > > > > > > > > > > > > > > > > > > > -- > > > > > Ryan Blue > > > > > Software Engineer > > > > > Netflix > > > > > > > > > > > > > > > > > > -- > > > Ryan Blue > > > Software Engineer > > > Netflix > > > > > > > > -- > Ryan Blue > Software Engineer > Netflix >
