JunRuiLee commented on PR #8819: URL: https://github.com/apache/paimon/pull/8819#issuecomment-5055633640
> 1. **[P2] Composite tables are silently downgraded to regular tables** > > * `TableDescriptorSerializer.java:67` only records `location + schema`. > * The `location()` and `schema()` methods of `FallbackReadFileStoreTable` and `ChainGroupReadTable` are directly delegated to the primary table, so the descriptors generated for composite tables are **exactly the same** as those for regular primary tables. > * After reconstruction on the Rust side, the partition-level merge/fallback semantics for dual branches are lost, which may result in silently skipping or incorrectly reading data. > * I wrote a temporary test to confirm that the JSON of fallback tables and regular tables is byte-for-byte identical. > * It is recommended that v1 at least explicitly reject these wrappers; `tableKind` and branch/composition information can be added in future versions. > 2. **[P2] Dynamic configurations and credentials are included in plain text within the descriptor** > > * `AbstractFileStoreTable.copyInternal` merges all dynamic options into `tableSchema.options`, which is then exported in its entirety by `TableDescriptorSerializer.java:67`. > * Testing confirmed that the supported `fs.s3a.secret.key=review-secret` appears verbatim in the JSON. > * The Rust FFI already accepts `storage_options` separately, so the descriptor should no longer carry AK/SK. > * It is recommended to use a persistent schema and include necessary dynamic read options via an explicit allowlist; filter out storage, catalog, and sensitive configurations. > > There is also a contract issue: the top-level `options` is a public field, but the Java producer always sets it to `null`, and Rust v1 explicitly ignores it; it is best to remove it or implement fail-fast behavior for non-null values to prevent users from mistakenly believing it will take effect. Thanks for the careful review, @JingsongLi! All three addressed: 1. Composite tables silently downgraded — Fixed. TableDescriptorSerializer.from now rejects DelegatedFileStoreTable (covers fallback / chain / privileged) with an UnsupportedOperationException, so a composite table can no longer be serialized as if it were a plain one. 2. Dynamic options / credentials leaked in plaintext — Fixed. The serializer now ships the persisted schema (table.schemaManager().schema(table.schema().id())) instead of table.schema(), so the dynamic options merged in by copyInternal (incl. fs.s3a.secret.key and the injected path) no longer appear in the descriptor. Storage credentials are passed to the reader out of band. Added a test asserting fs.s3a.secret.key does not appear in the JSON. 3. Misleading top-level options — Removed the field from the DTO entirely (it was always null and ignored by the reader). One addition on top of the review: instead of rejecting non-main branches, v1 now supports a specific branch of a plain table — the branch travels in a dedicated branch field (omitted for main), and since schemaManager() is branch-scoped the persisted schema is read from the branch. Composite/fallback/chain tables remain out of scope. The reader side (paimon-rust) will be updated to consume this in its own PR once this contract lands. -- 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]
