Re: LongLiteral to DateLiteral
https://github.com/apache/incubator-iceberg/pull/815 Thank you, Vlad On 2020/02/20 17:55:35, Ryan Blue wrote: > Sounds reasonable to me. Can you open a PR to add the conversion? > > On Wed, Feb 19, 2020 at 2:40 PM Vlad Rozov 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 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 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 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 > > 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 > > 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 > > > > 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 >
Re: LongLiteral to DateLiteral
Sounds reasonable to me. Can you open a PR to add the conversion? On Wed, Feb 19, 2020 at 2:40 PM Vlad Rozov 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 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 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 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 > 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 > 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 > > > 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
Re: LongLiteral to DateLiteral
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 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 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 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 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 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 > > 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 >
Re: LongLiteral to DateLiteral
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 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 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 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 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 > 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
Re: LongLiteral to DateLiteral
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 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 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 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 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 >
Re: LongLiteral to DateLiteral
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 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 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 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
Re: LongLiteral to DateLiteral
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 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 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 >
Re: LongLiteral to DateLiteral
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 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
