[GitHub] [iceberg] szehon-ho commented on pull request #4898: Core: Add schema_id to ContentFile/ManifestFile

[GitBox <gi...@apache.org>]

szehon-ho commented on PR #4898:
URL: https://github.com/apache/iceberg/pull/4898#issuecomment-1154471992

   Yea there is some limited discussion in this related issue, but I guess no good conclusions:  https://github.com/apache/iceberg/issues/2542.  
   
   Maybe a test writer that creates metadata files with all optional columns as null?  That way can test all the new columns at once.
   
   By the way,  change mostly looks good, and it's good you put the new field is optional to avoid the issue like the one mentioned.  I just dont know enough about the serialization/deserialization code to be sure if there are any other problems with previous serialized metadata, so was hoping to have a test to verify it.  Though can leave to @rdblue to approve if he is confident , and we can tackle the backward compat tests on the side.


-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org