kbendick commented on a change in pull request #1427:
URL: https://github.com/apache/iceberg/pull/1427#discussion_r487463702
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java
##########
@@ -277,7 +277,7 @@ public boolean dropNamespace(Namespace namespace) {
return true;
} catch (InvalidOperationException e) {
- throw new NamespaceNotEmptyException("Namespace " + namespace + " is not
empty. One or more tables exist.", e);
+ throw new NamespaceNotEmptyException(e, "Namespace %s is not empty. One
or more tables exist.", namespace);
Review comment:
Hooray for compile time safety! My inner scala developer is pleased. 😀
##########
File path:
api/src/main/java/org/apache/iceberg/exceptions/AlreadyExistsException.java
##########
@@ -19,14 +19,18 @@
package org.apache.iceberg.exceptions;
+import com.google.errorprone.annotations.FormatMethod;
Review comment:
Oh. Actually upon further inspection I can see that this function
`bindLiterralOperation` is only called once in this class (and in the entire
code base currently) and there's a null check prior to calling it. So shouldn't
be a concern at the moment 👍
##########
File path:
api/src/main/java/org/apache/iceberg/exceptions/AlreadyExistsException.java
##########
@@ -19,14 +19,18 @@
package org.apache.iceberg.exceptions;
+import com.google.errorprone.annotations.FormatMethod;
Review comment:
Question: is this something that we should shade (possibly in a follow
up PR)? I know that normally we shade thing like guava etc that the user might
have on their classpath. Is it reasonable to expect that behavior here?
I don't think this needs to be addressed in this PR, but something that
crossed my mind as I'm re-reviewing this.
##########
File path:
api/src/main/java/org/apache/iceberg/expressions/UnboundPredicate.java
##########
@@ -136,9 +136,8 @@ private Expression bindLiteralOperation(BoundTerm<T>
boundTerm) {
Literal<T> lit = literal().to(boundTerm.type());
if (lit == null) {
- throw new ValidationException(String.format(
- "Invalid value for conversion to type %s: %s (%s)",
- boundTerm.type(), literal().value(),
literal().value().getClass().getName()));
+ throw new ValidationException("Invalid value for conversion to type %s:
%s (%s)",
+ boundTerm.type(), literal().value(),
literal().value().getClass().getName());
Review comment:
So looking at the definition of the `literal()` function in this class,
it seems it's possible for it to return null.
I guess it's not a concern as we would get NPE on the above call at line 136
when trying to call `.to` if `literal()` returned `null` before even getting to
this part that calls `literal().value()`, but something I thought I'd bring up.
Perhaps something we might follow up on in another issue or possibly I just
missed the workflow that makes `literal()`'s result non-null by the time this
`bindLiteralOperration` is called.
----------------------------------------------------------------
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]
