You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "RussellSpitzer (via GitHub)" <gi...@apache.org> on 2026/08/25 01:02:36 UTC
[PR] Implict struct file type [iceberg]
RussellSpitzer opened a new pull request, #17808:
URL: https://github.com/apache/iceberg/pull/17808
(no comment)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, CORE, PARQUET: Implicit struct file type [iceberg]
Posted by "GGraziadei (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, CORE, PARQUET: Implicit struct file type [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #17808:
URL: https://github.com/apache/iceberg/pull/17808#discussion_r3854309702
##########
api/src/main/java/org/apache/iceberg/types/AssignFreshIds.java:
##########
@@ -74,21 +83,28 @@ public Type schema(Schema schema, Supplier<Type> future) {
@Override
public Type struct(Types.StructType struct, Iterable<Type> futures) {
+ if (struct.isFileType()) {
+ // nested fields are rebuilt from the new id assigned to the field that holds this type
+ return struct;
+ }
+
List<Types.NestedField> fields = struct.fields();
int length = struct.fields().size();
// assign IDs for this struct's fields first
List<Integer> newIds = Lists.newArrayListWithExpectedSize(length);
for (int i = 0; i < length; i += 1) {
- newIds.add(idFor(name(fields.get(i).fieldId())));
+ Types.NestedField field = fields.get(i);
+ newIds.add(idFor(name(field.fieldId()), field.type()));
Review Comment:
We have to start plumbing "Type" through again to handle "file" types
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, CORE, PARQUET: Implicit struct file type [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #17808:
URL: https://github.com/apache/iceberg/pull/17808#issuecomment-6391411804
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, CORE, PARQUET: Implicit struct file type [iceberg]
Posted by "github-actions[bot] (via GitHub) via issues" <is...@iceberg.apache.org>.
github-actions[bot] commented on PR #17808:
URL: https://github.com/apache/iceberg/pull/17808#issuecomment-6566042564
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, CORE, PARQUET: Implicit struct file type [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer closed pull request #17808: API, CORE, PARQUET: Implicit struct file type
URL: https://github.com/apache/iceberg/pull/17808
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, CORE, PARQUET: Implicit struct file type [iceberg]
Posted by "github-actions[bot] (via GitHub) via issues" <is...@iceberg.apache.org>.
github-actions[bot] closed pull request #17808: API, CORE, PARQUET: Implicit struct file type
URL: https://github.com/apache/iceberg/pull/17808
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, CORE, PARQUET: Implicit struct file type [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #17808:
URL: https://github.com/apache/iceberg/pull/17808#discussion_r3854301060
##########
api/src/main/java/org/apache/iceberg/types/AssignFreshIds.java:
##########
@@ -48,15 +48,24 @@ class AssignFreshIds extends TypeUtil.CustomOrderSchemaVisitor<Type> {
this.nextId = nextId;
}
- private int idFor(String fullName) {
+ private int idFor(String fullName, Type type) {
+ Integer existingId = baseId(fullName);
+ if (existingId != null) {
+ return existingId;
+ }
+
+ return type.isFileType() ? nextId.get(Types.FileType.NUM_NESTED_FIELDS) : nextId.get();
Review Comment:
When we assign ID's we have to burn ID's equal to the number of fields within the File type.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, CORE, PARQUET: Implicit struct file type [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #17808:
URL: https://github.com/apache/iceberg/pull/17808#discussion_r3854447881
##########
api/src/main/java/org/apache/iceberg/Schema.java:
##########
@@ -61,13 +61,14 @@ public class Schema implements Serializable {
@VisibleForTesting static final int DEFAULT_VALUES_MIN_FORMAT_VERSION = 3;
@VisibleForTesting
- static final Map<Type.TypeID, Integer> MIN_FORMAT_VERSIONS =
+ static final Map<Class<? extends Type>, Integer> MIN_FORMAT_VERSIONS =
Review Comment:
I really didn't want to have to do this, but since File type shares an ID with Struct Type we can't match on that. Matching on class is a little less clean but the other option is to add a new type for file ID and then change our code everywhere that checks whether something is a Struct by typeId
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, CORE, PARQUET: Implicit struct file type [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #17808:
URL: https://github.com/apache/iceberg/pull/17808#issuecomment-5805180024
Closing in favor of the separate-type design (design B), which the discussion on #17919 settled on.
This PR made `FileType` a subclass of `StructType` so that `typeId()` returned `STRUCT`. The alternative gives `file` its own `TypeID.FILE` and has visitors fall back to struct behavior by default, which keeps engine integrations working without a change while still letting code that cares distinguish a file from a struct.
That approach is being landed as a series of small PRs starting with #18229.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org