tanmayrauth commented on PR #1706:
URL: https://github.com/apache/iceberg-go/pull/1706#issuecomment-5227380074
Thanks for the thorough look, this was really helpful. Pushed fixes for all
of it.
The double-release one was a good catch. I hadn't clocked that writeFile
already releases everything in task.Batches, so my defer rec.Release() was just
doing it a second time. Doesn't show up under DefaultAllocator, but yeah, in
the checked-allocator test it was quietly going negative before AssertSize
ran, so that assertion wasn't really testing anything. Dropped it everywhere.
And you're right that the checked allocator can't actually see the
accumulator, that comment was overclaiming. It's plain Go fields and []byte,
nothing goes through Arrow there. Rewrote the comment to say what it really
covers (the
write pipeline releasing its buffers) and added the upper-bound check so
it matches the others.
Rest of the notes are in too: pointed the all-null test at PopulatesBounds
as the real positive guard, added the nested-geo caveat to newGeoTestWriter,
and left the antimeridian breadcrumb, since GeoBoundsXY throwing out xmin >
xmax is fine for geometry but would eat valid geography bounds if someone
reused it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
