markhoerth opened a new issue, #14429:
URL: https://github.com/apache/gravitino/issues/14429
### Version
main branch, commit bb634defdc5663a8124be5fca1c8d9e52ac096ac (2.0.0-SNAPSHOT)
### Describe what's wrong
A client commits a schema change through the Iceberg REST service (`POST
/iceberg/v1/{prefix}/namespaces/{ns}/tables/{t}` with `add-schema` and
`set-current-schema` updates) that Iceberg's schema evolution rules do not
allow:
- narrowing or unrelated type changes (long->int, float->int, double->float,
string->int, int->string)
- decimal scale changes or precision reductions (decimal(9,2)->decimal(9,3),
decimal(9,2)->decimal(8,2))
- making an optional column required
- adding a required column without a default
The user expects the commit to be rejected with a 4xx and the schema left
unchanged. Instead the commit returns 200 and the new schema becomes current.
Data files written before the change can then no longer be read: after
long->int a pyiceberg scan fails with `Cannot promote long to int`, and after
adding a required column it fails with `Field is required, and could not be
found in the file`.
The same change sent through the Gravitino API (`PUT
/api/metalakes/{ml}/catalogs/{c}/schemas/{s}/tables/{t}`) on an identical table
is refused with 400, so the two APIs over the same catalog disagree on which
schema changes are legal. The behavior shows up with format-version 2 and
format-version 3 tables.
Cause, in Gravitino's code:
-
`iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java`
`updateTable` dispatches at line 252 to
`iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableOperationExecutor.java`
`updateTable` (lines 118-130).
- The only check applied there to `AddSchema` updates is
`IcebergColumnFieldValidator.validateUpdate` (line 126). In
`iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergColumnFieldValidator.java`
lines 40-46 it validates column names and comments of the new schema only. The
new schema is never compared with the table's current schema.
- The request then goes unchanged to
`iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java`
`updateTable` (lines 351-360), which calls upstream
`CatalogHandlers.updateTable` at line 355. The federated path does the same at
`iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/FederatedCatalogWrapper.java:582`.
Upstream Iceberg's server side does not check either.
`CatalogHandlers.updateTable` applies the updates to `TableMetadata.Builder`,
whose `addSchema` only runs `Schema.checkCompatibility(schema, formatVersion)`:
a format-version check, not a check against the previous schema. Iceberg leaves
the promotion and nullability rules to the client-side `UpdateSchema`, so any
REST client that builds the schema itself can commit a schema that makes
existing files unreadable.
The Gravitino API refuses the change because
`catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/ops/IcebergCatalogWrapperHelper.java`
routes column changes through Iceberg's `UpdateSchema` (`updateColumn` at line
160, `requireColumn` at line 108, `addRequiredColumn` at line 177), which
rejects them.
A fix belongs in Gravitino's IRC commit path, for example next to the
existing `AddSchema` hook in `IcebergTableOperationExecutor.updateTable` /
`IcebergColumnFieldValidator.validateUpdate`: compare each added schema with
the current table schema by field id, require type changes to satisfy
`TypeUtil.isPromotionAllowed`, refuse optional to required changes, and require
an initial default for a new required field.
### Error message and/or stacktrace
IRC commit (long->int), HTTP 200; the current schema becomes:
```
{"type":"struct","schema-id":1,"fields":[{"id":1,"name":"l","required":false,"type":"int"},{"id":2,"name":"s","required":false,"type":"string"}]}
```
Gravitino API, same change on an identical table, HTTP 400:
```
{"code":1001,"type":"IllegalArgumentException","message":"Failed to operate
table(s) [gi] operation [ALTER] under schema [vrf_evo], reason [Cannot change
column type: l: long -> int]","stack":["java.lang.IllegalArgumentException:
Cannot change column type: l: long -> int","\tat
org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkArgument(Preconditions.java:484)","\tat
org.apache.iceberg.SchemaUpdate.updateColumn(SchemaUpdate.java:285)","\tat
org.apache.gravitino.catalog.lakehouse.iceberg.ops.IcebergCatalogWrapperHelper.doUpdateColumnType(IcebergCatalogWrapperHelper.java:160)",
...]}
```
IRC commit adding required column `nr`, HTTP 200; the new field is
`{"id":3,"name":"nr","required":true,"type":"string"}`. Gravitino API, same
change, HTTP 400:
```
{"code":1001,"type":"IllegalArgumentException","message":"Failed to operate
table(s) [gr] operation [ALTER] under schema [vrf_evo], reason [Incompatible
change: cannot add required column without a default value: nr]", ...}
```
Reading existing data after the IRC commits (pyiceberg 0.12.0, tables
holding two rows written before the change):
```
ResolveError: Cannot promote long to int
ResolveError: Field is required, and could not be found in the file: 3: nr:
required string
```
The same 200 versus 400 split occurs for float->int, double->float,
string->int, int->string, decimal(9,2)->decimal(9,3),
decimal(9,2)->decimal(8,2) and optional int to required int.
### How to reproduce
Precondition: the Iceberg REST service runs with
`gravitino.iceberg-rest.catalog-config-provider = dynamic-config-provider` and
`gravitino.iceberg-rest.gravitino-metalake = vrf_ml`, so the IRC prefix is the
Gravitino catalog name.
1. Create a metalake:
```
curl -X POST -H "Authorization: Basic YWRtaW46eA==" -H "Content-Type:
application/json" http://localhost:8090/api/metalakes -d
'{"name":"vrf_ml","comment":"","properties":{}}'
```
2. Create an Iceberg catalog:
```
curl -X POST -H "Authorization: Basic YWRtaW46eA==" -H "Content-Type:
application/json" http://localhost:8090/api/metalakes/vrf_ml/catalogs -d
'{"name":"vrf_evo_cat","type":"RELATIONAL","provider":"lakehouse-iceberg","comment":"","properties":{"catalog-backend":"jdbc","uri":"jdbc:h2:file:/tmp/vrf_evo_wh/h2;AUTO_SERVER=TRUE;DATABASE_TO_LOWER=TRUE","jdbc-driver":"org.h2.Driver","jdbc-user":"sa","jdbc-password":"","jdbc-initialize":"true","warehouse":"/tmp/vrf_evo_wh/wh"}}'
```
3. Create a namespace and two identical tables through the IRC:
```
curl -X POST -H "Authorization: Basic YWRtaW46eA==" -H "Content-Type:
application/json" http://localhost:9001/iceberg/v1/vrf_evo_cat/namespaces -d
'{"namespace":["vrf_evo"],"properties":{}}'
for t in li gi; do curl -X POST -H "Authorization: Basic YWRtaW46eA==" -H
"Content-Type: application/json"
http://localhost:9001/iceberg/v1/vrf_evo_cat/namespaces/vrf_evo/tables -d
'{"name":"'$t'","properties":{"format-version":"2"},"schema":{"type":"struct","schema-id":0,"fields":[{"id":1,"name":"l","required":false,"type":"long"},{"id":2,"name":"s","required":false,"type":"string"}]}}';
done
```
4. Change `l` from long to int through the IRC. The response is 200:
```
curl -X POST -H "Authorization: Basic YWRtaW46eA==" -H "Content-Type:
application/json"
http://localhost:9001/iceberg/v1/vrf_evo_cat/namespaces/vrf_evo/tables/li -d
'{"requirements":[{"type":"assert-current-schema-id","current-schema-id":0}],"updates":[{"action":"add-schema","schema":{"type":"struct","fields":[{"id":1,"name":"l","required":false,"type":"int"},{"id":2,"name":"s","required":false,"type":"string"}]},"last-column-id":2},{"action":"set-current-schema","schema-id":-1}]}'
```
5. Make the same change through the Gravitino API. The response is 400:
```
curl -X PUT -H "Authorization: Basic YWRtaW46eA==" -H "Content-Type:
application/json"
http://localhost:8090/api/metalakes/vrf_ml/catalogs/vrf_evo_cat/schemas/vrf_evo/tables/gi
-d
'{"updates":[{"@type":"updateColumnType","fieldName":["l"],"newType":"integer"}]}'
```
6. Adding a required column behaves the same way. Create tables `ar` and
`gr` as in step 3, then run both requests. The first returns 200, the second
400:
```
curl -X POST -H "Authorization: Basic YWRtaW46eA==" -H "Content-Type:
application/json"
http://localhost:9001/iceberg/v1/vrf_evo_cat/namespaces/vrf_evo/tables/ar -d
'{"requirements":[{"type":"assert-current-schema-id","current-schema-id":0}],"updates":[{"action":"add-schema","schema":{"type":"struct","fields":[{"id":1,"name":"l","required":false,"type":"long"},{"id":2,"name":"s","required":false,"type":"string"},{"id":3,"name":"nr","required":true,"type":"string"}]},"last-column-id":3},{"action":"set-current-schema","schema-id":-1}]}'
curl -X PUT -H "Authorization: Basic YWRtaW46eA==" -H "Content-Type:
application/json"
http://localhost:8090/api/metalakes/vrf_ml/catalogs/vrf_evo_cat/schemas/vrf_evo/tables/gr
-d
'{"updates":[{"@type":"addColumn","fieldName":["nr"],"type":"string","nullable":false}]}'
```
7. To see the read failure, append rows to a table with any Iceberg client
(for example pyiceberg against the REST catalog with `warehouse="vrf_evo_cat"`)
before step 4 or 6, then scan the table after the commit. The scan fails with
`Cannot promote long to int` or `Field is required, and could not be found in
the file`.
### Additional context
Related: #899 (closed, incompatible changes such as required columns through
the Gravitino API) and #11954 (closed, format-version 3 table creation). No
existing issue covers the missing validation on the Iceberg REST commit path.
The missing check is in Gravitino's IRC commit path, which forwards the
update to upstream `CatalogHandlers.updateTable`; upstream Iceberg's server
side does not validate schema evolution either, so Gravitino currently inherits
that gap.
Found by the 2.0 OSS test suite check
`test_schema_evolution_verdict_matches_gravitino` (parameters `long->int`,
`float->int`, `decimal(9,2)->decimal(9,3)`, `decimal(9,2)->decimal(8,2)`,
`string->int`, `int->string`, `double->float`, `int->required`, `add_required`).
--
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]
