Add more null checks to TableIdentifier #2703
Conversation
|
@rdblue is this something that you could potentially review? |
| private TableIdentifier(Namespace namespace, String name) { | ||
| Preconditions.checkArgument(name != null && !name.isEmpty(), "Invalid table name %s", name); | ||
| Preconditions.checkArgument(namespace != null, "Namespace must be non-null"); | ||
| Preconditions.checkArgument(name != null && !name.isEmpty(), "Table name must be non-null/non-empty"); |
There was a problem hiding this comment.
There's no need to move this check. Reordering without a clear reason just results in more commit conflicts later.
Also, the original error message conforms to our conventions better than the new one. Our convention is to be as direct as possible, which is why it started with "Invalid table name". If you'd like to add the "non-null/non-empty" part, I'd recommend using "Invalid table name (null or empty): %s".
There was a problem hiding this comment.
sure thing. I would probably just have Invalid table name (null or empty) / Invalid table name: null or empty (to align with other error messages) without printing what the actual name is, since it's already mentioned in the error message (null or empty)
There was a problem hiding this comment.
It's a minor distinction, but I would normally include what the user actually had. If it was null, then it will show up as Invalid table name (null or empty): null. Not a big deal the way it is here, though.
api/src/main/java/org/apache/iceberg/catalog/TableIdentifier.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/iceberg/catalog/TableIdentifier.java
Outdated
Show resolved
Hide resolved
nastra
force-pushed
the
table-identifier-improvements
branch
from
8bd133f to
c88fe3c
Compare
|
Thanks for the fix, @nastra! |
| private final String name; | ||
|
|
||
| public static TableIdentifier of(String... names) { | ||
| Preconditions.checkArgument(names != null, "Cannot create table identifier from null array"); |
There was a problem hiding this comment.
I don't think null check is needed for java varargs. The names will be empty array if no input is given. I verified it locally:
private void met(String... strs) {
System.out.println(Arrays.toString(strs));
}
met();
The result is []
There was a problem hiding this comment.
The problem isn't when this is used as a varargs call. It is that this actually creates of(String[] names) in the class file. So you can call it directly with a String array:
String[] levels = null;
return Namespace.of(levels); // failsThere was a problem hiding this comment.
Ah I see, never thought about this use case, thanks!
There was a problem hiding this comment.
there's also a test that verifies this behavior: https://github.com/apache/iceberg/pull/2703/files#diff-7d459f8dfa23098a11664c5f2399f8a8a6c27d72bfcf4384e9bb580fdaff0963R72
3 participants
No description provided.