GGraziadei commented on PR #17808: URL: https://github.com/apache/iceberg/pull/17808#issuecomment-5418180164
Hi @RussellSpitzer, hope it's okay to jump in here! I went through the PR and wanted to share some thoughts on the design. While making `File` structurally identical to a struct (physical group in parquet, fields mapping to URI/offset) makes total sense, having it `extends StructType` introduces a subtle trap: it asserts **identity**, not just representation. This leads to a classic **Liskov Substitution Principle (LSP)** violation—it compiles, but quietly breaks substitutability. We can already see the interest on this technical debt building up in the PR through: * The 4 `if (struct.isFileType())` early returns across the ID assignment visitors. * `MIN_FORMAT_VERSIONS` having to be keyed by `Class` instead of `TypeID`. * The `isFileType()` guard inside `StructType.equals`. These aren't features of `File`; they are exceptions carved out to bypass the inherited identity, which will make future folds and type checks fragile. **An alternative approach:** Instead of inheriting identity, we could use **delegation**. `File` can hold a `StructType` internally for field storage/lookup, and expose a small capability interface (e.g., `fields()`, `field(id)`, `withEnclosingId(int)`). This keeps the exact representation you want while eliminating the `equals` guard, the `assignedType()` plumbing, and all the early returns by centralizing the derived ID rule. Wdyt? -- 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]
