rdblue commented on code in PR #17513:
URL: https://github.com/apache/iceberg/pull/17513#discussion_r3770746237
##########
api/src/main/java/org/apache/iceberg/Schema.java:
##########
@@ -449,6 +457,39 @@ public String idToAlias(Integer fieldId) {
return null;
}
+ /**
+ * Returns whether the sub-field identified by the field id is effectively
optional.
+ *
+ * <p>A field is effectively optional if it is declared optional, or if it
is nested inside an
+ * optional field. For example, a required field inside an optional struct
is null whenever that
+ * struct is null. Field defaults are not taken into account, so an optional
field with a non-null
+ * default is still optional.
+ *
+ * @param id a field id
+ * @return true if the field may be null, false if it cannot be null
+ * @throws IllegalArgumentException if the field is not present in this
schema
+ */
+ public boolean isOptional(int id) {
Review Comment:
I do not think this method should not be part of the public API. I pointed
out a very similar problem on the other version of this method, here:
https://github.com/apache/iceberg/pull/17413/changes#r3762320465
Whether a field is optional is clearly defined: there's a boolean flag that
determines whether the field's value can be null. This method misuses the term
"optional" to mean whether the field or any parent may be null. That difference
is not clear and makes the term "optional" confusing. I also don't think that
this gets any better by introducing a different "nullable" term.
My point on the other PR wasn't to move this method to the public API, it
was that we should rename the utility method to be clear. To do that, I think
we should switch to using `required`. If the field and all parents are required
("fully required" or "always present"?) that is the opposite of this method.
Using a name like `alwaysPresent` combined with `isRequired` for the leaf makes
the most sense to me, but we can come up with better ideas.
--
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]
