Fix entity name overlap handling and tests (#4168)#4190
Fix entity name overlap handling and tests (#4168)#4190Subham-KRLX wants to merge 3 commits intoapache:mainfrom
Conversation
Subham-KRLX
force-pushed
the
fix/entity-name-overlap-tests-4168
branch
from
524576c to
d8f5928
Compare
Subham-KRLX
force-pushed
the
fix/entity-name-overlap-tests-4168
branch
from
d8f5928 to
985169f
Compare
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for working on this, @Subham-KRLX !
| Stream.of(PolarisEntitySubType.ICEBERG_TABLE, PolarisEntitySubType.ICEBERG_VIEW) | ||
| .filter( | ||
| subType -> | ||
| listTableLike(subType, parentNamespace, PageToken.readEverything()) |
There was a problem hiding this comment.
Can we avoid "list" operations in this case?
Would it be possible to "resolve" the name to be created for any potential previous type and then do the "already exists" check?
There was a problem hiding this comment.
I replaced the list based check with direct name lookup for table/view under the parent namespace and we still return the same AlreadyExistsException when a collision is found.
| } | ||
|
|
||
| private boolean namespaceWithSameNameExists(TableIdentifier identifier) { | ||
| return listNamespaces(identifier.namespace()).stream() |
There was a problem hiding this comment.
Actually, can we avoid "list" operations in this case too?
There was a problem hiding this comment.
Replaced the list based check with a direct name lookup for namespaces using readEntityByName to avoid the 'list' operation here too Verified the fix with RestCatalogFileIT
| List<PolarisEntity> catalogPath = resolvedEntities.getRawFullPath(); | ||
| EntityResult lookupResult = | ||
| getMetaStoreManager() | ||
| .readEntityByName( |
There was a problem hiding this comment.
Ideally we should do it when we resolve resolvedEntityView.
That is, we should add a resolution path with the new entity name for all possible entity types when the PolarisResolutionManifest is constructed and do the look up for all of those potential name matches together (via the Resolver). WDYT?
Is that feasible?
There was a problem hiding this comment.
That approach seems very efficient for batching lookups within the Resolver I am planning to refactor the implementation to pre load all potential conflicting entity types (NAMESPACE and TABLE_LIKE) as optional paths in the PolarisResolutionManifest during the CatalogHandler authorization phase. Would this strategy be consistent with what you have in mind? If so then IcebergCatalog can perform collision checks by directly calling resolvedEntityView.getResolvedPath() which eliminates redundant metadata store lookups and ensures all relevant names are resolved together by the Resolver as you suggested.
|
Thanks @Subham-KRLX for working on it. This a behavior change. It's worth to share it in the dev mailing list for more visibility. Would you mind share it there? Current behavior in the main branch:
What's NEW in this PR (behavior change):
|
|
I agree with @flyrain here. I think a discussion on the dev@ mailing list is appropriate and welcome. |
@flyrain @dimas-b I sent the mailing list proposal regarding Introduce DataSourceResolver for multi-datasource support in JDBC but have not received a response yet should we resume the review here to maintain momentum? Please let me know if there is another communication channel or community meeting where I should present this instead. |
|
@Subham-KRLX : I believe this change is valuable, but it is not related to your My understanding is that other reviewers prefer to have a separate Would you mind starting this new discussion on |
@dimas-b would it be okay to start the discussion in the #dev channel on Slack? It might be easier for a quick back-and-forth on the technical approach and 'easier' to maintain momentum once we align there I can post a summary of the outcome to the mailing list for the official record Would that work for you? |
|
@Subham-KRLX : slack is not "visible" from the ASF process perspective 🤷 Please start an email thread. If a particular topic becomes convoluted we can certainly use slack to resolve it faster, but ultimately all design decisions will have to be carried over to the Alternatively, I can send the initial email if you prefer - please let me know. |
4 participants
Fixes #4168 by adding shared integration tests for namespace/table/view name collisions and enforcing consistent AlreadyExistsException behavior across backends. Verified with RestCatalogFileIT and NoSqlCatalogIT.