Copilot commented on code in PR #14684:
URL: https://github.com/apache/gravitino/pull/14684#discussion_r4482793800
##########
server/src/main/java/org/apache/gravitino/server/web/rest/SemanticModelSourceValidator.java:
##########
@@ -190,6 +210,55 @@ private Set<String> loadColumns(
throw new IllegalSemanticModelException("Semantic Model source %s does not
exist", ident);
}
+ private static long registeredTableId(NameIdentifier ident, Table table) {
+ if (table instanceof EntityCombinedTable) {
+ EntityCombinedTable combined = (EntityCombinedTable) table;
+ if (combined.tableFromGravitino() != null) {
+ return combined.tableFromGravitino().id();
+ }
+ // Managed tables carry their stored ID in the loaded catalog
properties, without a separate
+ // TableEntity. Read it before public-property masking, without
resolving the name again.
+ if (combined.imported()) {
+ StringIdentifier id =
+
StringIdentifier.fromProperties(combined.tableFromCatalog().properties());
+ if (id != null) {
+ return id.id();
+ }
+ }
+ }
+ throw new IllegalStateException("Source table has no registered Gravitino
identity: " + ident);
+ }
+
+ private static long registeredViewId(NameIdentifier ident, View view) {
+ if (view instanceof EntityCombinedView) {
+ EntityCombinedView combined = (EntityCombinedView) view;
+ if (combined.viewEntity() != null) {
+ return combined.viewEntity().id();
+ }
+ // Managed views use the same stored-identity property as managed tables.
+ if (combined.imported()) {
+ StringIdentifier id =
+
StringIdentifier.fromProperties(combined.viewFromCatalog().properties());
+ if (id != null) {
+ return id.id();
+ }
+ }
+ }
+ throw new IllegalStateException("Source View has no registered Gravitino
identity: " + ident);
+ }
Review Comment:
The table/view identity resolution logic is nearly identical (entity-id
first, then imported-properties fallback, then throw). This duplication
increases the chance of future drift (e.g., adding a new fallback path for one
but not the other). Consider extracting a shared helper that takes
lambdas/accessors for `entity()`, `imported()`, and
`fromCatalog().properties()` so both paths stay consistent.
##########
core/src/main/java/org/apache/gravitino/catalog/SemanticModelDispatcher.java:
##########
@@ -126,37 +129,40 @@ SemanticModel createSemanticModel(
default SemanticModel importOssieDocument(
Namespace namespace,
OssieDocument document,
- Consumer<SemanticModelDefinition> sourceValidator) {
+ Function<SemanticModelDefinition, SemanticModelSourceUpdate>
sourceValidator) {
Objects.requireNonNull(sourceValidator, "sourceValidator must not be
null");
ParsedSemanticModel model =
OssieDocumentConverter.importDocument(document);
- sourceValidator.accept(model.definition());
+ SemanticModelSourceUpdate sourceUpdate =
sourceValidator.apply(model.definition());
return importSemanticModel(
NameIdentifier.of(namespace, model.name()),
model.comment(),
model.definition(),
- model.properties());
+ model.properties(),
+ sourceUpdate);
}
Review Comment:
The `sourceValidator` function’s return value is not validated. If an
implementation accidentally returns `null`, this will fail later with an
unclear NPE (potentially deeper in the stack). Consider adding an explicit null
check with a clear message (e.g., `Objects.requireNonNull(sourceUpdate, ...)`)
immediately after applying the function.
##########
server/src/test/java/org/apache/gravitino/server/web/rest/TestSemanticModelSourceValidator.java:
##########
@@ -91,12 +99,101 @@ void
testCrossCatalogSourceAndKeysWithDisabledAuthorization() {
}
@Test
- void testViewFallbackAndColumns() {
+ void testManagedTableUsesIdentityFromLoadedProperties() {
+ Table managed = mock(Table.class);
+ doReturn(columns("id")).when(managed).columns();
+ when(managed.properties())
+
.thenReturn(StringIdentifier.newPropertiesWithId(StringIdentifier.fromId(303L),
Map.of()));
+ EntityCombinedTable combined =
EntityCombinedTable.of(managed).withImported(true);
+ when(tables.loadTable(FULL_SOURCE)).thenReturn(combined);
+ SemanticModelSourceUpdate update =
+ validator.validate("metalake", definition(dataset("orders", null,
null)));
+ assertEquals(303L, update.sources().get(0).getSourceId());
+ assertEquals(EntityType.TABLE, update.sources().get(0).getSourceType());
+ verify(tables).loadTable(FULL_SOURCE);
+ verifyNoInteractions(views);
+
+ // An unregistered external table's connector property is not a registered
identity.
+ combined.withImported(false);
Review Comment:
If `withImported(false)` follows a common fluent/builder convention and
returns a new instance rather than mutating `combined`, this line won’t change
the object used by `tables.loadTable(...)`, making the assertion
ineffective/flake-prone. To make the test robust regardless of `withImported`
semantics, reassign the return value (or explicitly stub `imported()` / create
a new combined instance) before the second validation call.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/SemanticModelSourceValidator.java:
##########
@@ -190,6 +210,55 @@ private Set<String> loadColumns(
throw new IllegalSemanticModelException("Semantic Model source %s does not
exist", ident);
}
+ private static long registeredTableId(NameIdentifier ident, Table table) {
+ if (table instanceof EntityCombinedTable) {
+ EntityCombinedTable combined = (EntityCombinedTable) table;
+ if (combined.tableFromGravitino() != null) {
+ return combined.tableFromGravitino().id();
+ }
+ // Managed tables carry their stored ID in the loaded catalog
properties, without a separate
+ // TableEntity. Read it before public-property masking, without
resolving the name again.
+ if (combined.imported()) {
+ StringIdentifier id =
+
StringIdentifier.fromProperties(combined.tableFromCatalog().properties());
+ if (id != null) {
+ return id.id();
+ }
+ }
+ }
+ throw new IllegalStateException("Source table has no registered Gravitino
identity: " + ident);
+ }
+
+ private static long registeredViewId(NameIdentifier ident, View view) {
+ if (view instanceof EntityCombinedView) {
+ EntityCombinedView combined = (EntityCombinedView) view;
+ if (combined.viewEntity() != null) {
+ return combined.viewEntity().id();
+ }
+ // Managed views use the same stored-identity property as managed tables.
+ if (combined.imported()) {
+ StringIdentifier id =
+
StringIdentifier.fromProperties(combined.viewFromCatalog().properties());
+ if (id != null) {
+ return id.id();
+ }
+ }
+ }
+ throw new IllegalStateException("Source View has no registered Gravitino
identity: " + ident);
Review Comment:
Inconsistent capitalization in the error message (`Source View` vs `Source
table`). For consistency and professionalism in logs/errors, consider using
`Source view ...` (lowercase 'view') to match the table message style.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
