nvartolomei commented on code in PR #14004:
URL: https://github.com/apache/iceberg/pull/14004#discussion_r2333372794


##########
format/spec.md:
##########
@@ -1861,6 +1861,18 @@ Java writes `-1` for "no current snapshot" with V1 and 
V2 tables and considers t
 
 Some implementations require that GZIP compressed files have the suffix 
`.gz.metadata.json` to be read correctly. The Java reference implementation can 
additionally read GZIP compressed files with the suffix `metadata.json.gz`.  
 
+### Schema Evolution/Type Promotion
+
+Column projection rules are designed so that the table will remain readable 
even if writers use an outdated schema. Writers should bind the latest schema 
at the beginning of a transaction.  Note, that in the common cases of schema 
evolution (adding nullable columns, adding required columns with an 
`initial-default`, renaming a column, dropping a column, or doing type 
promotion) then appending data with outdated schemas presents no issues under 
either SNAPSHOT or SERIALIZABLE isolation levels.
+
+While writers are not required to bind to the latest schema there are edge 
cases to consider:
+
+1. Assume two transactions that are started concurrently. The first modifies 
the `write-default` on the column. The second is a data write that makes use of 
`write-default` from the changed column in the first transaction. If the first 
transaction gets committed first, the result of the second transaction depends 
on isolation level. Under SNAPSHOT isolation the second transaction can be 
committed. However, the second transaction produces the serialization anomaly 
of using the outdated `write-default` default value.  SERIALIZABLE isolation 
does not allow for such anomolies and the second transaction must fail in this 
mode. The transaction could be retried after updating to the new schema and 
rewriting the data using the new `write-default`.
+
+2. Assume a sequence of the linear transactions: the first transaction adds a 
columnand populates it with new values. The second transactions  is run using 
the schema prior to the new column being added and updates another column (e.g. 
`update table x set pre_existing_col='xyz'`). Transaction b) must fail under 
both SNAPSHOT and SERIALIZABLE isolation levels, since it would drop data from 
the new column added in the first transaction. If the transactions started 
concurrently, one of them should still fail with SNAPSHOT isolation because 
there is an overlap in the rows modified by the transactions.
+
+Writers must write out all fields with the types specified from the schema 
they bound to. Writing all fields prevents similar anomolies as those outlined 
for the first case above but with `initial-default` instead of `write-default` 
(all nulls can be distinguished from missing columns that need an initial 
default).

Review Comment:
   Again the `bound to` which I believe here means that writer must commit with 
an assertion on the schema. But without more precise wording/binding being 
defined I'm tempted to interpret as "use schema which was actual when 
transaction started". I'd prefer more precise language.



##########
format/spec.md:
##########
@@ -1861,6 +1861,18 @@ Java writes `-1` for "no current snapshot" with V1 and 
V2 tables and considers t
 
 Some implementations require that GZIP compressed files have the suffix 
`.gz.metadata.json` to be read correctly. The Java reference implementation can 
additionally read GZIP compressed files with the suffix `metadata.json.gz`.  
 
+### Schema Evolution/Type Promotion
+
+Column projection rules are designed so that the table will remain readable 
even if writers use an outdated schema. Writers should bind the latest schema 
at the beginning of a transaction.  Note, that in the common cases of schema 
evolution (adding nullable columns, adding required columns with an 
`initial-default`, renaming a column, dropping a column, or doing type 
promotion) then appending data with outdated schemas presents no issues under 
either SNAPSHOT or SERIALIZABLE isolation levels.

Review Comment:
   `bind` is a jargon which imho does't belong to the spec. Can we use a more 
clear language here? Do you mean here that the writer should inspect 
current-schema-id before creating data files and verify that current-schema-id 
didn't change during commit?



##########
format/spec.md:
##########
@@ -1861,6 +1861,18 @@ Java writes `-1` for "no current snapshot" with V1 and 
V2 tables and considers t
 
 Some implementations require that GZIP compressed files have the suffix 
`.gz.metadata.json` to be read correctly. The Java reference implementation can 
additionally read GZIP compressed files with the suffix `metadata.json.gz`.  
 
+### Schema Evolution/Type Promotion
+
+Column projection rules are designed so that the table will remain readable 
even if writers use an outdated schema. Writers should bind the latest schema 
at the beginning of a transaction.  Note, that in the common cases of schema 
evolution (adding nullable columns, adding required columns with an 
`initial-default`, renaming a column, dropping a column, or doing type 
promotion) then appending data with outdated schemas presents no issues under 
either SNAPSHOT or SERIALIZABLE isolation levels.
+
+While writers are not required to bind to the latest schema there are edge 
cases to consider:
+
+1. Assume two transactions that are started concurrently. The first modifies 
the `write-default` on the column. The second is a data write that makes use of 
`write-default` from the changed column in the first transaction. If the first 
transaction gets committed first, the result of the second transaction depends 
on isolation level. Under SNAPSHOT isolation the second transaction can be 
committed. However, the second transaction produces the serialization anomaly 
of using the outdated `write-default` default value.  SERIALIZABLE isolation 
does not allow for such anomolies and the second transaction must fail in this 
mode. The transaction could be retried after updating to the new schema and 
rewriting the data using the new `write-default`.

Review Comment:
   Shall we add a hint here that to implement serializable the writer must 
check current-schema-id and under snapshot it is free not to? 



##########
format/spec.md:
##########
@@ -1861,6 +1861,18 @@ Java writes `-1` for "no current snapshot" with V1 and 
V2 tables and considers t
 
 Some implementations require that GZIP compressed files have the suffix 
`.gz.metadata.json` to be read correctly. The Java reference implementation can 
additionally read GZIP compressed files with the suffix `metadata.json.gz`.  
 
+### Schema Evolution/Type Promotion
+
+Column projection rules are designed so that the table will remain readable 
even if writers use an outdated schema. Writers should bind the latest schema 
at the beginning of a transaction.  Note, that in the common cases of schema 
evolution (adding nullable columns, adding required columns with an 
`initial-default`, renaming a column, dropping a column, or doing type 
promotion) then appending data with outdated schemas presents no issues under 
either SNAPSHOT or SERIALIZABLE isolation levels.
+
+While writers are not required to bind to the latest schema there are edge 
cases to consider:
+
+1. Assume two transactions that are started concurrently. The first modifies 
the `write-default` on the column. The second is a data write that makes use of 
`write-default` from the changed column in the first transaction. If the first 
transaction gets committed first, the result of the second transaction depends 
on isolation level. Under SNAPSHOT isolation the second transaction can be 
committed. However, the second transaction produces the serialization anomaly 
of using the outdated `write-default` default value.  SERIALIZABLE isolation 
does not allow for such anomolies and the second transaction must fail in this 
mode. The transaction could be retried after updating to the new schema and 
rewriting the data using the new `write-default`.
+
+2. Assume a sequence of the linear transactions: the first transaction adds a 
columnand populates it with new values. The second transactions  is run using 
the schema prior to the new column being added and updates another column (e.g. 
`update table x set pre_existing_col='xyz'`). Transaction b) must fail under 
both SNAPSHOT and SERIALIZABLE isolation levels, since it would drop data from 
the new column added in the first transaction. If the transactions started 
concurrently, one of them should still fail with SNAPSHOT isolation because 
there is an overlap in the rows modified by the transactions.

Review Comment:
   * `of the linear` - i believe article is unnecessary here
   * `columnand` - missing space



##########
format/spec.md:
##########
@@ -1861,6 +1861,18 @@ Java writes `-1` for "no current snapshot" with V1 and 
V2 tables and considers t
 
 Some implementations require that GZIP compressed files have the suffix 
`.gz.metadata.json` to be read correctly. The Java reference implementation can 
additionally read GZIP compressed files with the suffix `metadata.json.gz`.  
 
+### Schema Evolution/Type Promotion
+
+Column projection rules are designed so that the table will remain readable 
even if writers use an outdated schema. Writers should bind the latest schema 
at the beginning of a transaction.  Note, that in the common cases of schema 
evolution (adding nullable columns, adding required columns with an 
`initial-default`, renaming a column, dropping a column, or doing type 
promotion) then appending data with outdated schemas presents no issues under 
either SNAPSHOT or SERIALIZABLE isolation levels.
+
+While writers are not required to bind to the latest schema there are edge 
cases to consider:
+
+1. Assume two transactions that are started concurrently. The first modifies 
the `write-default` on the column. The second is a data write that makes use of 
`write-default` from the changed column in the first transaction. If the first 
transaction gets committed first, the result of the second transaction depends 
on isolation level. Under SNAPSHOT isolation the second transaction can be 
committed. However, the second transaction produces the serialization anomaly 
of using the outdated `write-default` default value.  SERIALIZABLE isolation 
does not allow for such anomolies and the second transaction must fail in this 
mode. The transaction could be retried after updating to the new schema and 
rewriting the data using the new `write-default`.
+
+2. Assume a sequence of the linear transactions: the first transaction adds a 
columnand populates it with new values. The second transactions  is run using 
the schema prior to the new column being added and updates another column (e.g. 
`update table x set pre_existing_col='xyz'`). Transaction b) must fail under 
both SNAPSHOT and SERIALIZABLE isolation levels, since it would drop data from 
the new column added in the first transaction. If the transactions started 
concurrently, one of them should still fail with SNAPSHOT isolation because 
there is an overlap in the rows modified by the transactions.

Review Comment:
   I understand the scenario but seems a bit miss-placed/unnecessary here. This 
is more of a general problem of reading with the wrong schema rather than about 
evolution.
   
   I.e. you don't have to read the table with latest schema to avoid data loss.
   
   You have to read a snapshot with the snapshot's `schema-id` and you must 
`assert-ref-snapshot-id` during write.



##########
format/spec.md:
##########
@@ -1861,6 +1861,18 @@ Java writes `-1` for "no current snapshot" with V1 and 
V2 tables and considers t
 
 Some implementations require that GZIP compressed files have the suffix 
`.gz.metadata.json` to be read correctly. The Java reference implementation can 
additionally read GZIP compressed files with the suffix `metadata.json.gz`.  
 
+### Schema Evolution/Type Promotion
+
+Column projection rules are designed so that the table will remain readable 
even if writers use an outdated schema. Writers should bind the latest schema 
at the beginning of a transaction.  Note, that in the common cases of schema 
evolution (adding nullable columns, adding required columns with an 
`initial-default`, renaming a column, dropping a column, or doing type 
promotion) then appending data with outdated schemas presents no issues under 
either SNAPSHOT or SERIALIZABLE isolation levels.

Review Comment:
   Maybe define bind. Seems useful to have a concept to refer to in subsequent 
paragraphs.



##########
format/spec.md:
##########
@@ -1861,6 +1861,18 @@ Java writes `-1` for "no current snapshot" with V1 and 
V2 tables and considers t
 
 Some implementations require that GZIP compressed files have the suffix 
`.gz.metadata.json` to be read correctly. The Java reference implementation can 
additionally read GZIP compressed files with the suffix `metadata.json.gz`.  
 
+### Schema Evolution/Type Promotion
+
+Column projection rules are designed so that the table will remain readable 
even if writers use an outdated schema. Writers should bind the latest schema 
at the beginning of a transaction.  Note, that in the common cases of schema 
evolution (adding nullable columns, adding required columns with an 
`initial-default`, renaming a column, dropping a column, or doing type 
promotion) then appending data with outdated schemas presents no issues under 
either SNAPSHOT or SERIALIZABLE isolation levels.
+
+While writers are not required to bind to the latest schema there are edge 
cases to consider:
+
+1. Assume two transactions that are started concurrently. The first modifies 
the `write-default` on the column. The second is a data write that makes use of 
`write-default` from the changed column in the first transaction. If the first 
transaction gets committed first, the result of the second transaction depends 
on isolation level. Under SNAPSHOT isolation the second transaction can be 
committed. However, the second transaction produces the serialization anomaly 
of using the outdated `write-default` default value.  SERIALIZABLE isolation 
does not allow for such anomolies and the second transaction must fail in this 
mode. The transaction could be retried after updating to the new schema and 
rewriting the data using the new `write-default`.
+
+2. Assume a sequence of the linear transactions: the first transaction adds a 
columnand populates it with new values. The second transactions  is run using 
the schema prior to the new column being added and updates another column (e.g. 
`update table x set pre_existing_col='xyz'`). Transaction b) must fail under 
both SNAPSHOT and SERIALIZABLE isolation levels, since it would drop data from 
the new column added in the first transaction. If the transactions started 
concurrently, one of them should still fail with SNAPSHOT isolation because 
there is an overlap in the rows modified by the transactions.
+
+Writers must write out all fields with the types specified from the schema 
they bound to. Writing all fields prevents similar anomolies as those outlined 
for the first case above but with `initial-default` instead of `write-default` 
(all nulls can be distinguished from missing columns that need an initial 
default).

Review Comment:
   `all nulls can be distinguished`
   
   Did you mean `can't` or I'm reading this wrong?



-- 
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]

Reply via email to