kbendick commented on a change in pull request #2675:
URL: https://github.com/apache/iceberg/pull/2675#discussion_r645363982
##########
File path: api/src/main/java/org/apache/iceberg/ManifestFile.java
##########
@@ -179,6 +181,13 @@ default boolean hasDeletedFiles() {
*/
List<PartitionFieldSummary> partitions();
+ /**
+ * Returns metadata about how this manifest file is encrypted, or null if
the file is stored in plain text.
+ */
+ default ByteBuffer keyMetadata() {
+ return null;
+ }
Review comment:
Is there any way to avoid using `null` as the return argument for
`default` methods?
This particular case isn't too bad, as the `null` return value is documented
and has semantic meaning. But I've noticed that we return `null` for a number
of methods in base classes or default methods in interfaces, many times where
it's expected that the child classes _will_ override the method, that could
lead to bad results.
So partially I wanted to bring that issue up as it seems to be a code style
that is employed sometimes that is somewhat unsafe, and I've noticed it in a
few PRs recently.
[I recently fixed a
bug](https://github.com/apache/iceberg/pull/2630/files#diff-1ae8e9490fe1a4b6da8842c1c313fa57e68a674e186bfeace1c763bee1381faaL72-L74)
arising from a similar situation - in this case,
`BaseMetastoreTableOperations#tableName`, which should have been `abstract` as
all catalogs needed to implement it, that was subsequently not overridden by
`NessieTableOperations` and would have lead to all of the issues one can
imagine with unexpected nulls in a PR that was being merged at the time.
##########
File path: api/src/main/java/org/apache/iceberg/ManifestFile.java
##########
@@ -179,6 +181,13 @@ default boolean hasDeletedFiles() {
*/
List<PartitionFieldSummary> partitions();
+ /**
+ * Returns metadata about how this manifest file is encrypted, or null if
the file is stored in plain text.
+ */
+ default ByteBuffer keyMetadata() {
+ return null;
+ }
Review comment:
Having looked through the PR further, I think the usage of `null` here
is definitely justified. Especially as the utility methods in `ByteBuffers`
handle null already so there's not too much extra null handling, and the
physical type needs to be byte[] for Avro.
Feel free to resolve this, but I've seen this in a number of PRs recently
where the `null` return value was more just a stand in for `abstract` and I
felt it would be good to draw attention to this in general as it seems to be
creeping into the codebase here and there. However, this is not one of those
cases. 🙂
--
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]
