tanmayrauth commented on code in PR #1464:
URL: https://github.com/apache/iceberg-go/pull/1464#discussion_r3575999410
##########
manifest.go:
##########
@@ -1531,6 +1552,7 @@ func (m *ManifestListWriter) AddManifests(files
[]ManifestFile) error {
var tmp manifestFileV1
for _, file := range files {
file.(*manifestFile).toV1(&tmp)
+ ensurePartitionList(&tmp.PartitionList)
Review Comment:
The v1 branch fix here has no test that fails without it.
`TestManifestListV1WriterPartitionListEncoding` builds its manifests fresh via
`Partitions([]FieldSummary{})`, which stores a non-nil empty slice, so
`ensurePartitionList` is a no-op on that input — the test passes even with this
line removed. The v1 collapse only reproduces on a decode→rewrite (a read-back
empty array decodes to a nil slice), same as the v2 case the e2e test covers.
As written, a future refactor that drops this line reintroduces the
null-collapse for v1 manifest-list rewrites with the suite still green. A v1
analogue of
`TestManifestListWriterPreservesEmptyPartitionArrayWhenRewritingInheritedManifest`
— read a v1 manifest list back, then rewrite it as v1, would close the gap.
--
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]
