aokolnychyi commented on a change in pull request #1890:
URL: https://github.com/apache/iceberg/pull/1890#discussion_r539145887
##########
File path:
spark3/src/main/java/org/apache/iceberg/spark/procedures/BaseProcedure.java
##########
@@ -74,26 +71,23 @@ protected BaseProcedure(TableCatalog tableCatalog) {
return result;
}
- // we have to parse both namespace and name as they may be quoted
- protected Identifier toIdentifier(String namespaceAsString, String name) {
- String[] namespaceParts = parseMultipartIdentifier(namespaceAsString);
+ protected Identifier toIdentifier(String identifierAsString, String argName)
{
+ Preconditions.checkArgument(identifierAsString != null &&
!identifierAsString.isEmpty(),
+ "Cannot handle an empty identifier for argument %s", argName);
- String[] nameParts = parseMultipartIdentifier(name);
- Preconditions.checkArgument(nameParts.length == 1, "Name must consist of
one part: %s", name);
-
- return Identifier.of(namespaceParts, nameParts[0]);
- }
-
- private String[] parseMultipartIdentifier(String identifierAsString) {
+ CatalogAndIdentifier catalogAndIdentifier;
try {
- ParserInterface parser = spark.sessionState().sqlParser();
- Seq<String> namePartsSeq =
parser.parseMultipartIdentifier(identifierAsString);
- String[] nameParts = new String[namePartsSeq.size()];
- namePartsSeq.copyToArray(nameParts);
- return nameParts;
+ catalogAndIdentifier = Spark3Util.catalogAndIdentifier(spark,
identifierAsString, tableCatalog);
} catch (ParseException e) {
- throw new RuntimeException("Couldn't parse identifier: " +
identifierAsString, e);
+ throw new IllegalArgumentException(String.format("Cannot parse
identifier '%s' for argument %s",
Review comment:
I like Ryan's suggestion in another PR to add a variant of
`catalogAndIdentifier` that does not throw a checked parse exception. Explicit
handling of parse exceptions makes other places more complicated.
If we follow that idea and add this method to `Spark3Util`:
```
public static CatalogAndIdentifier catalogAndIdentifier(String
description, SparkSession spark,
String name,
CatalogPlugin defaultCatalog) {
try {
return catalogAndIdentifier(spark, name, defaultCatalog);
} catch (ParseException e) {
throw new IllegalArgumentException("Cannot parse " + description + ":
" + name, e);
}
}
```
This place can look like this:
```
protected Identifier toIdentifier(String identifierAsString, String
argName) {
Preconditions.checkArgument(identifierAsString != null &&
!identifierAsString.isEmpty(),
"Cannot handle an empty identifier for argument %s", argName);
CatalogAndIdentifier catalogAndIdentifier =
Spark3Util.catalogAndIdentifier(
"identifier for arg " + argName, spark, identifierAsString,
tableCatalog);
CatalogPlugin catalog = catalogAndIdentifier.catalog();
Identifier identifier = catalogAndIdentifier.identifier();
Preconditions.checkArgument(
catalog.equals(tableCatalog),
"Cannot run procedure in catalog %s: %s is a table in catalog %s",
tableCatalog.name(), identifierAsString, catalog);
return identifier;
}
```
----------------------------------------------------------------
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]
