Wenjun7J commented on PR #16208: URL: https://github.com/apache/iceberg/pull/16208#issuecomment-4562803186
> LGTM, a good catch! > > I have verified the correctness premise: `PartitionData.copyFor(partition)` goes through the private `PartitionData(toCopy, partition)` copy constructor, which builds a fresh `data[]` from the new partition's values but shares `partitionType`, `size`, `stringSchema`, and `schema` references. All four shared fields are immutable in this context, so reusing one template across all `copyFor()` calls is safe. The `isSameAs` assertion in the new test is the right one for proving the optimization actually fires. > > Given that the core method of toPartitionData that previously returned a new object, now modifies the template is private, there is no external API exposure to worry about. > > However, have two small suggestions (see the previous comment and the followup) > > Also worth considering is adding the benchmarking from the description in the PR, or as a followup PR. Thanks for reviewing and for the detailed validation. The two suggestions are very helpful. I’ll address them, and may spend some time adding the benchmark in a follow-up PR. -- 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]
