roryqi commented on PR #14552: URL: https://github.com/apache/gravitino/pull/14552#issuecomment-6599218957
Reviewed at `2d9d54a22fbbe720340e8d21dbd104f5fac6a944`. Polaris currently explicitly marks `/plan` and `/tasks` as [not implemented](https://github.com/apache/polaris/blob/5b1fdfced8fa1f0929f4837c9772e384fb6e46e6/spec/polaris-catalog-service.yaml#L144), so I checked the behavior against Iceberg 1.11.0 instead. I reproduced the following issues using this PR's helper and Iceberg's serializers, parser, and evaluator: 1. **[P1] The rewritten filter leaks snapshot column names into the returned residual.** In the fallback path, a request for current column `l2` is rewritten to snapshot column `l`, but the resulting task's `residual-filter` is never translated back. If the current schema reuses `l` for a different field ID, the client filters the wrong column. Reproduction: request `l2 = 10`, current row `(l2=10, l=null)`; after serializing and parsing the task, its residual is `l = 10`, which evaluates to false instead of true. Without name reuse, binding the old name can fail instead. The returned residual must retain the request schema's field identities. See [filter rewriting](https://github.com/apache/gravitino/blob/2d9d54a22fbbe720340e8d21dbd104f5fac6a944/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergScanPlanHelper.java#L192). 2. **[P1] Sanitizing the server's spec map does not fix client deserialization.** `withSpecsById(...)` supplies serialization context; the repaired schemas are not transmitted in the response. The Iceberg REST client [parses using its own `table.specs()`](https://github.com/apache/iceberg/blob/apache-iceberg-1.11.0/core/src/main/java/org/apache/iceberg/rest/RESTTableScan.java#L112), which still contains `unknown` for the dropped source. An identity-partitioned file successfully serializes with the sanitized map, but parsing that JSON with the original current-table spec map throws `UnsupportedOperationException: Type: unknown is not supported`. The dropped-source fix needs a client round-trip regression test, beyond checking the helper's repaired map. 3. **[P2] A branch moving changes whether a valid current-schema filter is accepted.** With `use-snapshot-schema=false`, filtering an added column is accepted when a branch still points at the requested snapshot. Once no branch points there, `rewriteFilterNames` rejects it. For example, `isNull(newColumn)` throws `Cannot filter on field ...: it does not exist in snapshot ...`. The same snapshot/request should retain current-schema semantics regardless of subsequent branch movement. See [the rejection](https://github.com/apache/gravitino/blob/2d9d54a22fbbe720340e8d21dbd104f5fac6a944/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergScanPlanHelper.java#L183). 4. **[P2] Partition result types are used as source types, breaking mixed transforms and even empty results.** For a historical spec `identity(s), hour(ts)` after both sources are dropped, restoring `ts` from the hour partition's result type (`int`) causes `Unsupported type: int`. With no returned files, the binary fallback instead causes `Unsupported type: binary`. I also verified that the same empty response serializes successfully without sanitization: traversing all historical specs introduces a new failure for an empty scan. Source types need to come from the original source schema, rather than transform output types or a binary guess. See [type restoration](https://github.com/apache/gravitino/blob/2d9d54a22fbbe720340e8d21dbd104f5fac6a944/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergScanPlanHelper.java#L258). 5. **[P2] Multiple transforms of one dropped source produce duplicate schema fields.** A historical spec `identity(s), bucket(s, 8)` adds `_dropped_source_1` twice with the same source ID, producing `Invalid schema: multiple fields for name _dropped_source_1`. Restoration needs to deduplicate by source ID. See [the additions](https://github.com/apache/gravitino/blob/2d9d54a22fbbe720340e8d21dbd104f5fac6a944/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergScanPlanHelper.java#L262). There is also a smaller projection regression: `select: ["*"]`, which Iceberg's schema selection supports, is treated as a literal field name by the fallback rewrite and throws `Cannot find field '*' in the current table schema`. Validation scope: an isolated Java reproduction against the unmodified PR helper and Iceberg 1.11.0, including JSON serialization/deserialization and residual evaluation. I did not run the full Gradle suite or an HTTP integration test. The current tests' file-path assertions miss the residual and client parsing failures above. -- 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]
