Fix correctness gap in NoSQL hasOverlappingSiblings for parent locations

[vigneshio]

Summary

Fix correctness gap in NoSqlMetaStore.hasOverlappingSiblings so parent namespace location overlaps are now properly detected (matching the JDBC implementation).

Why this is needed

The NoSQL version only started the locations index iterator at the target key and only inspected the first result. Because of NUL-separated key ordering, parent locations were completely skipped. This could silently bypass the OPTIMIZED_SIBLING_CHECK validation.

Changes

• Added explicit prefix lookups for parent locations.
• Iterate the full matching range for children instead of only the first item.
• Added a regression test case for the parent-overlap scenario.

Checklist

• Don't disclose security issues! (contact security@apache.org)
• Clearly explained why the changes are needed, or linked related issues: N/A (internal correctness bug)
• Added/updated tests with good coverage, or manually tested (and explained how)
• Added comments for complex logic: N/A (straightforward)
• Updated CHANGELOG.md (if needed): N/A (internal backend fix)
• Updated documentation in site/content/in-dev/unreleased (if needed): N/A

[snazy]

This looks like the right fix for the NoSQL index lookup itself.

One non-blocking test suggestion: could we add an end-to-end/service-level test with OPTIMIZED_SIBLING_CHECK enabled? The current regression test exercises hasOverlappingSiblings directly, which covers the core bug, but the user-visible invariant is enforced through LocalIcebergCatalog.validateNoLocationOverlap. A test that creates an existing namespace/table at a parent location and then verifies that creating a child-location entity is rejected with the optimized path enabled would make the coverage match the actual bypass this fixes.

[vigneshio]

This looks like the right fix for the NoSQL index lookup itself.

One non-blocking test suggestion: could we add an end-to-end/service-level test with OPTIMIZED_SIBLING_CHECK enabled? The current regression test exercises hasOverlappingSiblings directly, which covers the core bug, but the user-visible invariant is enforced through LocalIcebergCatalog.validateNoLocationOverlap. A test that creates an existing namespace/table at a parent location and then verifies that creating a child-location entity is rejected with the optimized path enabled would make the coverage match the actual bypass this fixes.

Thnks @snazy .. I Added parent/child overlap assertions to the existing OPTIMIZED_SIBLING_CHECK=true test and a dedicated testParentChildOverlapWithOptimizedSiblingCheck that goes through the catalog + flag..

[snazy]

Thanks for adding the service-level coverage.

One concern: I don't think the new test actually exercises the NoSQL implementation changed in this PR. IcebergOverlappingTableTest uses TestServices.builder().config(...), and TestServices always builds an InMemoryPolarisMetaStoreManagerFactory, which uses the transactional/tree-map metastore rather than NoSqlMetaStore.

So this does cover the LocalIcebergCatalog.validateNoLocationOverlap service path with OPTIMIZED_SIBLING_CHECK enabled, but it would likely also pass without the NoSQL fix because the in-memory transactional backend already scans all entities for parent/child overlaps.

For this PR, I think the useful end-to-end regression would need to run the same create-table/create-namespace path with a NoSQL-backed test profile, for example using the existing NoSQL in-memory profile/config. Otherwise the direct TestNoSqlMetaStoreManager test is still the only test that covers the fixed NoSQL lookup logic.

[vigneshio]

Thanks for adding the service-level coverage.

One concern: I don't think the new test actually exercises the NoSQL implementation changed in this PR. IcebergOverlappingTableTest uses TestServices.builder().config(...), and TestServices always builds an InMemoryPolarisMetaStoreManagerFactory, which uses the transactional/tree-map metastore rather than NoSqlMetaStore.

So this does cover the LocalIcebergCatalog.validateNoLocationOverlap service path with OPTIMIZED_SIBLING_CHECK enabled, but it would likely also pass without the NoSQL fix because the in-memory transactional backend already scans all entities for parent/child overlaps.

For this PR, I think the useful end-to-end regression would need to run the same create-table/create-namespace path with a NoSQL-backed test profile, for example using the existing NoSQL in-memory profile/config. Otherwise the direct TestNoSqlMetaStoreManager test is still the only test that covers the fixed NoSQL lookup logic.

@snazy correct 💯 - IcebergOverlappingTableTest uses the transactional in-memory backend, so it doesn't cover NoSqlMetaStore. Now Added LocalIcebergCatalogNoSqlOverlapTest with a custom NoSQL profile (Profiles.NoSqlIcebergCatalogProfile) and overlap enforcement , so it exercises the fixed NoSqlMetaStore.hasOverlappingSiblings path through the service layer..

[snazy]

Thanks, LGTM

[dimas-b]

LGTM 👍

@snazy : WDYT?