Code Coverage Report

@jerryshao @yuqi1129 Could u help me review this pull request?

Is it ready to review?

Is it ready to review?

Yes, it is ready to review.

The overall direction here is good — extracting per-entity PO logic into dedicated classes and using a decorator for hierarchical name translation is a clean separation. A few observations on the design that might be worth considering.

The core tension

BasePOStorageOps.getPO/listPOs currently combines two orthogonal concerns:

1. Name translation (hierarchical logical ↔ physical) — handled well by HierarchicalConventionPOStorageOps
2. Query routing (cache-enabled → ID-based; no cache → full-name JOIN) — currently embedded via GravitinoEnv.getInstance().cacheEnabled() in BasePOStorageOps

Mixing these in the base class causes the Capability enum + runtime UnsupportedOperationException pattern, the GravitinoEnv static dependency leaking into a storage layer class, and the issue in MetadataObjectService.TYPE_TO_STORAGE_OPS_MAP where raw *POStorageOps instances bypass the naming convention.

An alternative: expose two explicit SQL methods, route in the service

// BasePOStorageOps: pure SQL delegation, no routing logic
abstract class BasePOStorageOps<PO, Mapper> {
    // ID-based (cache-enabled path)
    public PO getPOByParentId(Mapper mapper, Long parentId, String name) { ... }
    public List<PO> listPOsByParentId(Mapper mapper, Long parentId) { ... }

    // Name-based (full join, no-cache path)
    public PO getPOByFullName(Mapper mapper, NameIdentifier ident) { ... }
    public List<PO> listPOsByNSFullName(Mapper mapper, Namespace ns) { ... }
}

Routing moves back to the service as a single private helper per operation — not repeated per-method:

// SchemaMetaService
private SchemaPO getSchemaPO(NameIdentifier ident) {
    return SessionUtils.getWithoutCommit(SchemaMetaMapper.class, mapper ->
        cacheEnabled()
            ? ops.getPOByParentId(mapper, getCatalogId(ident), ident.name())
            : ops.getPOByFullName(mapper, ident));
}

Since ops is still a HierarchicalConventionPOStorageOps, both paths get name translation automatically. MetadataObjectService can store wrapped instances in its map instead of raw ops, fixing the naming convention bypass.

This eliminates the Capability enum, the GravitinoEnv dependency in the base class, and the dispatcher getPO(Mapper, NameIdentifier). Each layer has one job: name translation in the decorator, SQL in the ops, routing in the service.

What stays the same

HierarchicalConventionPOStorageOps as a decorator and the *POStorageOps concrete classes are the right abstraction — they just shouldn't know about cache state.

The overall direction here is good — extracting per-entity PO logic into dedicated classes and using a decorator for hierarchical name translation is a clean separation. A few observations on the design that might be worth considering.

The core tension

BasePOStorageOps.getPO/listPOs currently combines two orthogonal concerns:

1. Name translation (hierarchical logical ↔ physical) — handled well by HierarchicalConventionPOStorageOps
2. Query routing (cache-enabled → ID-based; no cache → full-name JOIN) — currently embedded via GravitinoEnv.getInstance().cacheEnabled() in BasePOStorageOps

Mixing these in the base class causes the Capability enum + runtime UnsupportedOperationException pattern, the GravitinoEnv static dependency leaking into a storage layer class, and the issue in MetadataObjectService.TYPE_TO_STORAGE_OPS_MAP where raw *POStorageOps instances bypass the naming convention.

An alternative: expose two explicit SQL methods, route in the service

// BasePOStorageOps: pure SQL delegation, no routing logic
abstract class BasePOStorageOps<PO, Mapper> {
    // ID-based (cache-enabled path)
    public PO getPOByParentId(Mapper mapper, Long parentId, String name) { ... }
    public List<PO> listPOsByParentId(Mapper mapper, Long parentId) { ... }

    // Name-based (full join, no-cache path)
    public PO getPOByFullName(Mapper mapper, NameIdentifier ident) { ... }
    public List<PO> listPOsByNSFullName(Mapper mapper, Namespace ns) { ... }
}

Routing moves back to the service as a single private helper per operation — not repeated per-method:

// SchemaMetaService
private SchemaPO getSchemaPO(NameIdentifier ident) {
    return SessionUtils.getWithoutCommit(SchemaMetaMapper.class, mapper ->
        cacheEnabled()
            ? ops.getPOByParentId(mapper, getCatalogId(ident), ident.name())
            : ops.getPOByFullName(mapper, ident));
}

Since ops is still a HierarchicalConventionPOStorageOps, both paths get name translation automatically. MetadataObjectService can store wrapped instances in its map instead of raw ops, fixing the naming convention bypass.

This eliminates the Capability enum, the GravitinoEnv dependency in the base class, and the dispatcher getPO(Mapper, NameIdentifier). Each layer has one job: name translation in the decorator, SQL in the ops, routing in the service.

What stays the same

HierarchicalConventionPOStorageOps as a decorator and the *POStorageOps concrete classes are the right abstraction — they just shouldn't know about cache state.

I got your point.
I will try solve this issue.
I want to extract a pattern that resolve the duplicated code for cached operations. Maybe I should extract a helper class for them.