Skip to content

Added typeCode to BasePersistence.lookupEntity #1112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 7, 2025
Merged

Added typeCode to BasePersistence.lookupEntity #1112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7bf8462
Added typeCode to BasePersistence.lookupEntity
dimas-b Mar 5, 2025
0a73252
review: clarify javadoc
dimas-b Mar 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...olaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ public void deleteAll(@Nonnull PolarisCallContext callCtx) {
/** {@inheritDoc} */
@Override
public @Nullable PolarisBaseEntity lookupEntity(
@Nonnull PolarisCallContext callCtx, long catalogId, long entityId) {
@Nonnull PolarisCallContext callCtx, long catalogId, long entityId, int typeCode) {
Copy link
Contributor

@singhpk234 singhpk234 Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please elaborate on how are you planning to use this in your NoSQL solution.
is your NoSQL solution still a single entity table ?
or are you using this to hint NoSQL on which index to use ?

Copy link
Contributor Author

@dimas-b dimas-b Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the latter

Copy link
Contributor Author

@dimas-b dimas-b Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not a hint though, it is a required input for finding right index for lookup (current POC, which is not published yet)

Copy link
Contributor

@singhpk234 singhpk234 Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, looking forward to your datamodel.

return ModelEntity.toEntity(this.store.lookupEntity(localSession.get(), catalogId, entityId));
}

Expand Down
13 changes: 10 additions & 3 deletions polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,17 +147,24 @@ void deleteAllEntityGrantRecords(

/**
* Lookup an entity given its catalog id (which can be {@link
* org.apache.polaris.core.entity.PolarisEntityConstants#NULL_ID} for top-level entities) and its
* entityId.
* org.apache.polaris.core.entity.PolarisEntityConstants#NULL_ID} for top-level entities), its
* entityId and type code (from {@link PolarisEntityType#getCode()}.
*
* <p>The type code parameter is redundant but can be used to optimize implementations in some
* cases. All callers are required to provide a valid value for the type code parameter. If the
* given type code does not match the type code of the previously created entity with the
* specified {@code entityId}, implementations may still return the entity or may behave as if the
* entity were not found.
*
* @param callCtx call context
* @param catalogId catalog id or NULL_ID
* @param entityId entity id
* @param typeCode the PolarisEntityType code of the entity to lookup
* @return null if the entity was not found, else the retrieved entity.
*/
@Nullable
PolarisBaseEntity lookupEntity(
@Nonnull PolarisCallContext callCtx, long catalogId, long entityId);
@Nonnull PolarisCallContext callCtx, long catalogId, long entityId, int typeCode);

/**
* Lookup an entity given its catalogId, parentId, typeCode, and name.
Expand Down
11 changes: 9 additions & 2 deletions polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,11 @@ DropEntityResult dropEntityIfExists(
* @param entityId the id of the entity to load
*/
@Nonnull
EntityResult loadEntity(@Nonnull PolarisCallContext callCtx, long entityCatalogId, long entityId);
EntityResult loadEntity(
@Nonnull PolarisCallContext callCtx,
long entityCatalogId,
long entityId,
@Nonnull PolarisEntityType entityType);
Copy link
Contributor

@dennishuo dennishuo Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should document in the javadoc whether this is intended to only be a "hint" or whether there's expectations that the type is enforced strictly. In particular, since we've defined entityId to need to be unique within the realm, if the underlying impl happens be able to look up the entity without checking entityType we should mention if it's safe for that impl to simpy ignore this param.

Due to the nature of this, I would prefer to let it be entityTypeHint and allow impls to ignore it, and distinguish the intended behavior from the strict entityType in readEntityByName.

Copy link
Contributor

@eric-maynard eric-maynard Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, if it can be ignored why are we even passing it in?

Copy link
Contributor

@eric-maynard eric-maynard Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a situation where we wouldn't want to enforce that entityType matches, even if the impl technically could look up an entity without it?

It seems to me that entityCatalogId is very similar -- if entity ID is unique across catalogs, technically you could look up the entity without entityCatalogId. But we still enforce it right?

Copy link
Contributor Author

@dimas-b dimas-b Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point it is a required parameter for the incoming NoSQL implementation.

Is that acceptable, @dennishuo ? If yes, I'll update the javadoc.

Copy link
Contributor Author

@dimas-b dimas-b Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, from my POV, it is perfectly fine for an API to have redundant inputs. Those can be used for validation as well as for optimization.

As for validation, the current behaviour expectation is to "not find" the object if the given type parameter does not match actual type in storage. I will update javadoc about that if we agree on adding the new parameter.


/**
* Fetch a list of tasks to be completed. Tasks
Expand Down Expand Up @@ -327,7 +331,10 @@ ChangeTrackingResult loadEntitiesChangeTracking(
*/
@Nonnull
ResolvedEntityResult loadResolvedEntityById(
@Nonnull PolarisCallContext callCtx, long entityCatalogId, long entityId);
@Nonnull PolarisCallContext callCtx,
long entityCatalogId,
long entityId,
PolarisEntityType entityType);
Copy link
Contributor

@singhpk234 singhpk234 Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[doubt] when can this be null ?

Copy link
Contributor Author

@dimas-b dimas-b Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, assuming non-null and annotating @Nullable cases is generally easier to maintain, but let's not open this can of worms in this PR. I'd welcome a dev list discussion on this, though :)

Copy link
Contributor

@adutra adutra Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to open a discuss thread about default nullability assumptions in polaris. I was asked to do so a while ago but forgot completely 😬


/**
* Load a resolved entity, i.e. an entity definition and associated grant records, from the
Expand Down
13 changes: 11 additions & 2 deletions ...c/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,10 @@ public ChangeTrackingResult loadEntitiesChangeTracking(

@Override
public EntityResult loadEntity(
@Nonnull PolarisCallContext callCtx, long entityCatalogId, long entityId) {
@Nonnull PolarisCallContext callCtx,
long entityCatalogId,
long entityId,
PolarisEntityType entityType) {
callCtx.getDiagServices().fail("illegal_method_in_transaction_workspace", "loadEntity");
return null;
}
Expand All @@ -320,13 +323,15 @@ public ScopedCredentialsResult getSubscopedCredsForEntity(
@Nonnull PolarisCallContext callCtx,
long catalogId,
long entityId,
PolarisEntityType entityType,
boolean allowListOperation,
@Nonnull Set<String> allowedReadLocations,
@Nonnull Set<String> allowedWriteLocations) {
return delegate.getSubscopedCredsForEntity(
callCtx,
catalogId,
entityId,
entityType,
allowListOperation,
allowedReadLocations,
allowedWriteLocations);
Expand All @@ -337,6 +342,7 @@ public ValidateAccessResult validateAccessToLocations(
@Nonnull PolarisCallContext callCtx,
long catalogId,
long entityId,
PolarisEntityType entityType,
@Nonnull Set<PolarisStorageActions> actions,
@Nonnull Set<String> locations) {
callCtx
Expand All @@ -347,7 +353,10 @@ public ValidateAccessResult validateAccessToLocations(

@Override
public ResolvedEntityResult loadResolvedEntityById(
@Nonnull PolarisCallContext callCtx, long entityCatalogId, long entityId) {
@Nonnull PolarisCallContext callCtx,
long entityCatalogId,
long entityId,
PolarisEntityType entityType) {
callCtx
.getDiagServices()
.fail("illegal_method_in_transaction_workspace", "loadResolvedEntityById");
Expand Down
10 changes: 7 additions & 3 deletions polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ && isNewer(existingCacheEntry, existingCacheEntryByName)) {
// try to load it
refreshedCacheEntry =
this.polarisMetaStoreManager.loadResolvedEntityById(
callContext, entityCatalogId, entityId);
callContext, entityCatalogId, entityId, entityType);
if (refreshedCacheEntry.isSuccess()) {
entity = refreshedCacheEntry.getEntity();
grantRecords = refreshedCacheEntry.getEntityGrantRecords();
Expand Down Expand Up @@ -361,7 +361,10 @@ && isNewer(existingCacheEntry, existingCacheEntryByName)) {
* entity, either as found in the cache or loaded from the backend
*/
public @Nullable EntityCacheLookupResult getOrLoadEntityById(
@Nonnull PolarisCallContext callContext, long entityCatalogId, long entityId) {
@Nonnull PolarisCallContext callContext,
long entityCatalogId,
long entityId,
PolarisEntityType entityType) {

// if it exists, we are set
ResolvedPolarisEntity entry = this.getEntityById(entityId);
Expand All @@ -374,7 +377,8 @@ && isNewer(existingCacheEntry, existingCacheEntryByName)) {

// load it
ResolvedEntityResult result =
polarisMetaStoreManager.loadResolvedEntityById(callContext, entityCatalogId, entityId);
polarisMetaStoreManager.loadResolvedEntityById(
callContext, entityCatalogId, entityId, entityType);

// not found, exit
if (!result.isSuccess()) {
Expand Down
4 changes: 2 additions & 2 deletions polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1027,7 +1027,7 @@ private ResolvedPolarisEntity resolveById(
if (this.cache != null) {
// get or load by name
EntityCacheLookupResult lookupResult =
this.cache.getOrLoadEntityById(this.polarisCallContext, catalogId, entityId);
this.cache.getOrLoadEntityById(this.polarisCallContext, catalogId, entityId, entityType);

// if not found, return null
if (lookupResult == null) {
Expand All @@ -1049,7 +1049,7 @@ private ResolvedPolarisEntity resolveById(
// If no cache, load directly from metastore manager.
ResolvedEntityResult result =
polarisMetaStoreManager.loadResolvedEntityById(
this.polarisCallContext, catalogId, entityId);
this.polarisCallContext, catalogId, entityId, entityType);
if (!result.isSuccess()) {
// not found
return null;
Expand Down
6 changes: 5 additions & 1 deletion ...a/org/apache/polaris/core/persistence/transactional/AbstractTransactionalPersistence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,11 @@ public PolarisBaseEntity lookupEntityByName(

// lookup the entity, should be there
PolarisBaseEntity entity =
lookupEntity(callCtx, entityActiveRecord.getCatalogId(), entityActiveRecord.getId());
lookupEntity(
callCtx,
entityActiveRecord.getCatalogId(),
entityActiveRecord.getId(),
entityActiveRecord.getTypeCode());
callCtx
.getDiagServices()
.checkNotNull(
Expand Down
Loading