Copilot commented on code in PR #13763:
URL: https://github.com/apache/gravitino/pull/13763#discussion_r4246110863
##########
server/src/main/java/org/apache/gravitino/server/web/rest/SemanticModelOperations.java:
##########
@@ -354,6 +355,10 @@ public Response alterSemanticModel(
request.getUpdates().stream()
.map(SemanticModelUpdateRequest::semanticModelChange)
.toArray(SemanticModelChange[]::new);
+ if (Arrays.stream(changes)
+ .anyMatch(change -> change instanceof
SemanticModelChange.ReplaceDefinition)) {
+ dispatcher.loadSemanticModel(ident);
+ }
for (SemanticModelChange change : changes) {
if (change instanceof SemanticModelChange.ReplaceDefinition) {
Review Comment:
This introduces an additional full pass over changes (stream anyMatch)
before the for-loop. While likely small, it’s avoidable: you can fold the
'loadSemanticModel once' check into the existing loop (e.g., on first
ReplaceDefinition) to eliminate the extra iteration and keep control flow in
one place.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/SemanticModelSourceValidator.java:
##########
@@ -144,32 +157,34 @@ private Set<String> loadColumns(
AuthorizationExpressionConstants.LOAD_TABLE_AUTHORIZATION_EXPRESSION,
currentAuthorizer,
context);
- if (tableAllowed) {
- try {
- return columnNames(ident, tables.loadTable(ident).columns());
- } catch (NotFoundException missing) {
- // The untyped source can also refer to a logical view. Authorize that
lookup separately.
- }
- }
boolean viewAllowed =
canLoad(
ident,
Entity.EntityType.VIEW,
AuthorizationExpressionConstants.LOAD_VIEW_AUTHORIZATION_EXPRESSION,
currentAuthorizer,
context);
- boolean viewSupported = true;
- if (tableAllowed || viewAllowed) {
- try {
- viewSupported = supportsViews.test(ident);
- if (viewAllowed && viewSupported) {
- return columnNames(ident, views.loadView(ident).columns());
+ if (!tableAllowed && !viewAllowed) {
+ throw new ForbiddenException("Not authorized to resolve Semantic Model
source %s", ident);
+ }
+ Set<Entity.EntityType> supported = EnumSet.of(Entity.EntityType.TABLE,
Entity.EntityType.VIEW);
+ try {
+ supported = supportedSourceTypes.apply(ident);
+ if (tableAllowed && supported.contains(Entity.EntityType.TABLE)) {
+ try {
+ return columnNames(ident, tables.loadTable(ident).columns());
+ } catch (NotFoundException missing) {
+ // An untyped source can also refer to a view.
}
- } catch (NotFoundException missing) {
- // Do not disclose whether the other, inaccessible object type exists.
}
+ if (viewAllowed && supported.contains(Entity.EntityType.VIEW)) {
+ return columnNames(ident, views.loadView(ident).columns());
+ }
Review Comment:
supportedSourceTypes.apply(ident) is executed per source resolution (i.e.,
per dataset). If a semantic model contains many datasets in the same catalog,
this can cause repeated catalog capability lookups. Consider memoizing
capability results keyed by catalog identifier for the duration of a single
validate() call (or wrapping supportedSourceTypes with a small cache) so
repeated sources in the same catalog don’t re-query capabilities.
--
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]
