Sounds reasonable to me. Can you open a PR to add the conversion? On Wed, Feb 19, 2020 at 2:40 PM Vlad Rozov <vro...@apache.org> wrote:
> 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 > > > -- Ryan Blue Software Engineer Netflix
