peach12345 commented on code in PR #14932:
URL: https://github.com/apache/iceberg/pull/14932#discussion_r2727031271
##########
api/src/main/java/org/apache/iceberg/catalog/TableIdentifier.java:
##########
@@ -26,10 +27,10 @@
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
/** Identifies a table in iceberg catalog. */
-public class TableIdentifier {
+public class TableIdentifier implements Serializable {
Review Comment:
The parsing function is, of course, working as intended.
However, in the CatalogTableLoader class, when you pass in a
TableIdentifier, the identifier is stored as a string:
```
private CatalogTableLoader(CatalogLoader catalogLoader, TableIdentifier
tableIdentifier) {
this.catalogLoader = catalogLoader;
this.identifier = tableIdentifier.toString();
}
```
Later, when calling the loadTable function, that string is parsed again.
This fails when the table name contains dots:
```
public Table loadTable() {
FlinkEnvironmentContext.init();
return catalog.loadTable(TableIdentifier.parse(identifier));
return catalog.loadTable(identifier);
}
```
That’s why storing the full TableIdentifier object instead of its string
representation makes more sense.
Fixing the parsing function to properly handle dots would also be a valid
approach, but in my experience, relying on parsing often leads to subtle errors
or bugs.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
