ajantha-bhat commented on code in PR #8672:
URL: https://github.com/apache/iceberg/pull/8672#discussion_r1339898288


##########
format/spec.md:
##########
@@ -128,13 +128,13 @@ Tables do not require rename, except for tables that use 
atomic rename to implem
 
 #### Writer requirements
 
-Some tables in this spec have columns that specify requirements for v1 and v2 
tables. These requirements are intended for writers when adding metadata files 
to a table with the given version.
+Some tables in this spec have columns that specify requirements for v1 and v2 
tables. These requirements are intended for writers when adding 
metadata/manifest files to a table with the given version.
 
-| Requirement | Write behavior |
-|-------------|----------------|
-| (blank)     | The field should be omitted |
-| _optional_  | The field can be written |
-| _required_  | The field must be written |
+| Requirement | Write behavior                                        |
+|-------------|-------------------------------------------------------|
+| (blank)     | The field should not be present in the schema         |

Review Comment:
   I think we cannot mention schema? Because Table metadata is a JSON document 
and we no where keep JSON schema of that document. For Avro it makes sense. But 
this section is generic I guess. 



##########
format/spec.md:
##########
@@ -128,13 +128,13 @@ Tables do not require rename, except for tables that use 
atomic rename to implem
 
 #### Writer requirements
 
-Some tables in this spec have columns that specify requirements for v1 and v2 
tables. These requirements are intended for writers when adding metadata files 
to a table with the given version.
+Some tables in this spec have columns that specify requirements for v1 and v2 
tables. These requirements are intended for writers when adding 
metadata/manifest files to a table with the given version.

Review Comment:
   manifest list file also need to be mentioned? 
   
   I thought previously `metadata` word is used as generic word for all Iceberg 
metadata files not table metadata here.  



##########
format/spec.md:
##########
@@ -128,13 +128,13 @@ Tables do not require rename, except for tables that use 
atomic rename to implem
 
 #### Writer requirements
 
-Some tables in this spec have columns that specify requirements for v1 and v2 
tables. These requirements are intended for writers when adding metadata files 
to a table with the given version.
+Some tables in this spec have columns that specify requirements for v1 and v2 
tables. These requirements are intended for writers when adding 
metadata/manifest files to a table with the given version.
 
-| Requirement | Write behavior |
-|-------------|----------------|
-| (blank)     | The field should be omitted |
-| _optional_  | The field can be written |
-| _required_  | The field must be written |
+| Requirement | Write behavior                                        |
+|-------------|-------------------------------------------------------|
+| (blank)     | The field should not be present in the schema         |
+| _optional_  | The field should be in the schema, and can be written |
+| _required_  | The field should in the schema and must be written    |

Review Comment:
   In the new texts added is it `should` or `must` of all these 3?  (Assuming 
we are discussing for Avro. In future these metadata or partition stats can be 
in parquet/orc also)



-- 
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-unsubscr...@iceberg.apache.org

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


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

Reply via email to