You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@iceberg.apache.org by Brian Hulette <bh...@apache.org> on 2024/04/04 00:45:12 UTC
truncate transform over binary columns
Hello, this is my first time writing on this list so I'll introduce
myself. I'm Brian Hulette, I've been involved with a couple of Apache
projects in the past (Arrow and Beam), and now I'm working on BigQuery's
support for Iceberg.
My colleague raised an issue [1] a while ago about a discrepancy between
the specification and the implementation - truncate is not supposed to work
on binary column, but it looks like it does. It seems unlikely that we
will drop support for something that is working now, so I figured we should
document the current behavior instead. I drafted a PR for this [2], could
someone help review?
Thanks!
Brian
[1] https://github.com/apache/iceberg/issues/5251
[2] https://github.com/apache/iceberg/pull/10079
Re: truncate transform over binary columns
Posted by Amogh Jahagirdar <am...@tabular.io>.
Another aspect that just occurred to me, there could be concern on just
directly updating the spec as is since it's ultimately the source of truth
(not the reference implementation). If there's concern that such a change
would make other implementations technically non-compliant because they
don't support truncate on binary, then another option is to say that
truncate binary is only officially supported as part of the v3 spec (with a
note that technically in the reference implementation it works prior to
v3). This feels a bit overkill to me for this case since if I look at the
history of this, it's been supported since the very beginning of the
reference implementation and I think it was a miss on the original spec
definition. However, if people are concerned about directly updating the
spec as is, then I think this is another option.
Thanks,
Amogh Jahagirdar
On Wed, Apr 3, 2024 at 9:36 PM Amogh Jahagirdar <am...@tabular.io> wrote:
> Thanks for bringing this up Brian! In my view, the decision tree for this
> would look something like:
>
> 1.) Is there anything incorrect with supporting truncate on the basis of
> width for binary columns? I can't really think of any reason, it seems
> legitimate to me (handling characters outside of utf-8 comes to mind) and I
> see engines like Spark already handle such a transform (for Spark I see
> TestSparkTruncateFunction#testTruncateBinary already validating the
> behavior). If there is something problematic with supporting this then I
> think we'd want to consider deprecating that behavior but again I don't see
> anything problematic since the transform is well defined.
>
> 2.) If there's nothing incorrect, then changing the spec makes sense. I
> left a review on the PR, the only thing is with a spec we want to be
> explicit so we should specify that truncate on binary will take [0, width)
> bytes (essentially the same as the string case).
>
> Right now, I'm favoring 2 but I think it's worth leaving this open for a
> bit in case others see an issue with supporting this behavior.
>
> Thanks,
>
> Amogh Jahagirdar
>
> On Wed, Apr 3, 2024 at 6:45 PM Brian Hulette <bh...@apache.org> wrote:
>
>> Hello, this is my first time writing on this list so I'll introduce
>> myself. I'm Brian Hulette, I've been involved with a couple of Apache
>> projects in the past (Arrow and Beam), and now I'm working on BigQuery's
>> support for Iceberg.
>>
>> My colleague raised an issue [1] a while ago about a discrepancy between
>> the specification and the implementation - truncate is not supposed to work
>> on binary column, but it looks like it does. It seems unlikely that we
>> will drop support for something that is working now, so I figured we should
>> document the current behavior instead. I drafted a PR for this [2], could
>> someone help review?
>>
>> Thanks!
>> Brian
>>
>> [1] https://github.com/apache/iceberg/issues/5251
>> [2] https://github.com/apache/iceberg/pull/10079
>>
>
Re: truncate transform over binary columns
Posted by Amogh Jahagirdar <am...@tabular.io>.
Thanks for bringing this up Brian! In my view, the decision tree for this
would look something like:
1.) Is there anything incorrect with supporting truncate on the basis of
width for binary columns? I can't really think of any reason, it seems
legitimate to me (handling characters outside of utf-8 comes to mind) and I
see engines like Spark already handle such a transform (for Spark I see
TestSparkTruncateFunction#testTruncateBinary already validating the
behavior). If there is something problematic with supporting this then I
think we'd want to consider deprecating that behavior but again I don't see
anything problematic since the transform is well defined.
2.) If there's nothing incorrect, then changing the spec makes sense. I
left a review on the PR, the only thing is with a spec we want to be
explicit so we should specify that truncate on binary will take [0, width)
bytes (essentially the same as the string case).
Right now, I'm favoring 2 but I think it's worth leaving this open for a
bit in case others see an issue with supporting this behavior.
Thanks,
Amogh Jahagirdar
On Wed, Apr 3, 2024 at 6:45 PM Brian Hulette <bh...@apache.org> wrote:
> Hello, this is my first time writing on this list so I'll introduce
> myself. I'm Brian Hulette, I've been involved with a couple of Apache
> projects in the past (Arrow and Beam), and now I'm working on BigQuery's
> support for Iceberg.
>
> My colleague raised an issue [1] a while ago about a discrepancy between
> the specification and the implementation - truncate is not supposed to work
> on binary column, but it looks like it does. It seems unlikely that we
> will drop support for something that is working now, so I figured we should
> document the current behavior instead. I drafted a PR for this [2], could
> someone help review?
>
> Thanks!
> Brian
>
> [1] https://github.com/apache/iceberg/issues/5251
> [2] https://github.com/apache/iceberg/pull/10079
>
Re: truncate transform over binary columns
Posted by Brian Hulette <bh...@apache.org>.
>
> Quick question: do you actually have an issue with truncate on binary
> columns ?
No issue - as a consumer of Iceberg metadata I'd just like to clarify if we
should expect to see partition fields with truncated binary. I was
initially coding against the spec and planned to reject a truncate
transform over anything but int, long, decimal, or string, until the
TruncateByteBuffer support was brought to my attention. To Amogh's point -
I suppose it's possible other consumers have already made that decision and
would be broken by changing this in the spec. But I agree it feels like
overkill to put this in v3, it does seem like it was just a miss in the
original spec.
On Thu, Apr 4, 2024 at 6:19 AM Jean-Baptiste Onofré <jb...@nanthrax.net> wrote:
> Hi Brian,
>
> Welcome to this list :)
>
> Quick question: do you actually have an issue with truncate on binary
> columns ? The Truncate transform (in Iceberg API) supports BINARY
> using TruncateByteBuffer, so it would make sense to clearly state this
> in the spec.
>
> Regards
> JB
>
> On Thu, Apr 4, 2024 at 2:45 AM Brian Hulette <bh...@apache.org> wrote:
> >
> > Hello, this is my first time writing on this list so I'll introduce
> myself. I'm Brian Hulette, I've been involved with a couple of Apache
> projects in the past (Arrow and Beam), and now I'm working on BigQuery's
> support for Iceberg.
> >
> > My colleague raised an issue [1] a while ago about a discrepancy between
> the specification and the implementation - truncate is not supposed to work
> on binary column, but it looks like it does. It seems unlikely that we
> will drop support for something that is working now, so I figured we should
> document the current behavior instead. I drafted a PR for this [2], could
> someone help review?
> >
> > Thanks!
> > Brian
> >
> > [1] https://github.com/apache/iceberg/issues/5251
> > [2] https://github.com/apache/iceberg/pull/10079
>
Re: truncate transform over binary columns
Posted by Jean-Baptiste Onofré <jb...@nanthrax.net>.
Hi Brian,
Welcome to this list :)
Quick question: do you actually have an issue with truncate on binary
columns ? The Truncate transform (in Iceberg API) supports BINARY
using TruncateByteBuffer, so it would make sense to clearly state this
in the spec.
Regards
JB
On Thu, Apr 4, 2024 at 2:45 AM Brian Hulette <bh...@apache.org> wrote:
>
> Hello, this is my first time writing on this list so I'll introduce myself. I'm Brian Hulette, I've been involved with a couple of Apache projects in the past (Arrow and Beam), and now I'm working on BigQuery's support for Iceberg.
>
> My colleague raised an issue [1] a while ago about a discrepancy between the specification and the implementation - truncate is not supposed to work on binary column, but it looks like it does. It seems unlikely that we will drop support for something that is working now, so I figured we should document the current behavior instead. I drafted a PR for this [2], could someone help review?
>
> Thanks!
> Brian
>
> [1] https://github.com/apache/iceberg/issues/5251
> [2] https://github.com/apache/iceberg/pull/10079