rdblue commented on a change in pull request #3722:
URL: https://github.com/apache/iceberg/pull/3722#discussion_r768092154
##########
File path:
spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/source/IcebergSource.java
##########
@@ -115,10 +133,28 @@ public Table getTable(StructType schema, Transform[]
partitioning, Map<String, S
if (catalogAndIdentifier.catalog().name().equals("spark_catalog") &&
!(catalogAndIdentifier.catalog() instanceof SparkSessionCatalog)) {
// catalog is a session catalog but does not support Iceberg. Use
Iceberg instead.
+ Identifier ident = catalogAndIdentifier.identifier();
return new
Spark3Util.CatalogAndIdentifier(catalogManager.catalog(DEFAULT_CATALOG_NAME),
- catalogAndIdentifier.identifier());
- } else {
+ newIdentifier(ident, selector));
+ } else if (snapshotId == null && asOfTimestamp == null) {
Review comment:
I think this mixes the identifier logic in too many places. The
identifier should be independent of the `if`/`else` blocks here that determine
catalog. We don't need to check the selector in `newIdentifier` as well as the
data that the selector is dependent on here.
Instead, I think this should create the new identifier using
`identifierWithSelector(catalogAndIdentifier.identifier(), selector)` and then
pass that identifier through this block.
--
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]
