[GitHub] [spark] cloud-fan edited a comment on issue #25077: [SPARK-28301][SQL] fix the behavior of table name resolution with multi-catalog

[GitBox <gi...@apache.org>]

cloud-fan edited a comment on issue #25077: [SPARK-28301][SQL] fix the behavior of table name resolution with multi-catalog
URL: https://github.com/apache/spark/pull/25077#issuecomment-509472675
 
 
   The reasons that I open this PR:
   1. The fix in #24768 leads to different behaviors than I expect: a) temp view should have higher priority. b) table provider should not affect table name resolution. The implementation also needs to be different for the proposed behaviors, that's one reason I open this PR.
   2. #24768 does 2 things together: 1. fix the table name resolution. 2. support v2 provider with Hive catalog. Since discussion is needed for 1, I think it's more efficient to have an individual PR for it. Since the implementation in my mind is different, I open a new PR instead of asking you to separate the changes to a new PR.
   
   > but this PR makes the v1 SessionCatalog responsible for proxying calls to load v2 tables by modifying the signature and behavior of lookupRelation.
   
   The `lookupRelation` should belong to analyzer instead of catalog, since catalog doesn't know the relation plans Spark uses. We put it in `SessionCatalog` for historical reasons, I can move it to analyzer in a new PR if it's better. **To clarify, I agree with you that v1 session catalog should not proxy calls to v2 catalogs.**
   
   >  But this PR rewrites AsTableIdentifier so that it will match any 2-part identifier without the catalog check so it now must be applied after CatalogObjectIdentifier.
   
   Thanks for catching this bug! I feel it's cleaner to call `AsTableIdentifier` after `CatalogObjectIdentifier`, but we must guarantee that when `AsTableIdentifier` is called, no v2 catalog can be found. I've pushed a commit to fix it.
   
   Note that, the main point of this PR is to trigger discussion and reach a consensus about the behavior of table name resolution, including all the details. Once we reach a consensus, we can merge your PR first as long as it matches the expected behavior (you can take some tests in this PR), and rebase my PR as a refactor.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org