kbendick opened a new pull request #1491: URL: https://github.com/apache/iceberg/pull/1491
The class PruneColumns has a single constructor, which takes in a Set<Integer>, `PruneColumns(Set<Integer> selected)`. Later on in this class, there are no null checks before calling methods on the value `selected` such as `contains`, etc. Admittedly, the only place that PruneColumns is _currently_ used, `org.apache.iceberg.types.TypeUtils#select(Types.StructType struct, Set<Integer> fieldIds)`, has a precondition check for null on fieldIds - which is passed into PruneColumns. Thus, it is arguably not necessary to add this additional check. However, because the class `PruneColumns` is part of `api` and might get used elsewhere, it seems sane to add this precondition check given that we do no null checking before accessing it and so it would be up to the caller to add the preconditions check if they were to reuse it. Lastly, it's important to note that `PruneColumns`, while part of `api`, is not declared as Public and so one could argue that this is not needed given that it's internal to iceberg and anybody using it should just know that its constructor argument must be non-null. I'm not a personal believer in tribal knowledge like that, and so I'm opening this PR. If we don't think this PR is necessary, given the precondition check in the one place that it is currently used, then we can go ahead and close this PR. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
