jerryshao commented on code in PR #14147:
URL: https://github.com/apache/gravitino/pull/14147#discussion_r4408536312
##########
clients/client-python/gravitino/api/semantic/semantic_model_catalog.py:
##########
@@ -55,6 +56,45 @@ def list_semantic_models(self, namespace: Namespace) ->
list[NameIdentifier]:
NoSuchSchemaException: If the schema does not exist.
"""
+ def list_metrics(self, namespace: Namespace) -> list[NameIdentifier]:
+ """List visible metric identifiers across the Semantic Models in a
schema.
+
+ Identifiers are relative to this catalog. Each identifier's namespace
+ contains the schema and Semantic Model names, and its name is the exact
+ metric name.
+ Results are ordered by model identifier and then case-sensitive metric
name.
+
+ Args:
+ namespace (Namespace): A single-level schema namespace.
+
+ Returns:
+ list[NameIdentifier]: The visible metric identifiers, or an empty
list.
+
+ Raises:
+ NoSuchSchemaException: If the schema does not exist.
+ UnsupportedOperationException: If metric discovery is not
supported.
+ """
+ raise UnsupportedOperationException("Metric discovery is not
supported")
+
+ def list_metric_infos(self, namespace: Namespace) ->
list[SemanticModelMetric]:
+ """List complete visible metrics across the Semantic Models in a
schema.
+
+ Each result contains a namespace of schema and model names and the
complete
+ metric definition. Visibility and ordering match :meth:`list_metrics`.
+ The request fails as a whole if metric definitions cannot be decoded.
+
+ Args:
+ namespace (Namespace): A single-level schema namespace.
+
+ Returns:
+ list[SemanticModelMetric]: The visible metrics, or an empty list.
+
+ Raises:
+ NoSuchSchemaException: If the schema does not exist.
+ UnsupportedOperationException: If metric discovery is not
supported.
+ """
+ raise UnsupportedOperationException("Metric discovery is not
supported")
Review Comment:
[Nit] Both defaults raise the identical message `"Metric discovery is not
supported"` (here and at line 77), so a caller that catches
`UnsupportedOperationException` cannot tell which API was unsupported. The Java
counterparts distinguish them: `"Metric listing is not supported"` and `"Metric
info listing is not supported"`
(`api/src/main/java/org/apache/gravitino/semantic/SemanticModelCatalog.java:57`
and `:72`). Consider matching that wording so the two languages report the same
thing.
Verified by: reading both new default methods in this file and the two Java
defaults in `SemanticModelCatalog.java`;
`tests/unittests/api/semantic/test_semantic_model_catalog.py` asserts the
shared message for both methods, so it would need the same split.
##########
clients/client-python/gravitino/client/semantic_model_catalog_operations.py:
##########
@@ -86,6 +88,42 @@ def list_semantic_models(self, namespace: Namespace) ->
list[NameIdentifier]:
for ident in entity_list_resp.identifiers()
]
+ def list_metrics(self, namespace: Namespace) -> list[NameIdentifier]:
+ self._check_semantic_model_namespace(namespace)
+ resp = self._rest_client.get(
+ self._format_metric_request_path(
+ self._get_semantic_model_full_namespace(namespace)
+ ),
+ params={"details": "false"},
+ error_handler=SEMANTIC_MODEL_ERROR_HANDLER,
+ )
+ response = EntityListResponse.from_json(resp.body, infer_missing=True)
+ response.validate()
+ return [
+ NameIdentifier.builder(
+ self._relative_metric_namespace(ident.namespace()),
ident.name()
+ )
+ for ident in response.identifiers()
+ ]
+
+ def list_metric_infos(self, namespace: Namespace) ->
list[SemanticModelMetric]:
+ self._check_semantic_model_namespace(namespace)
+ resp = self._rest_client.get(
+ self._format_metric_request_path(
+ self._get_semantic_model_full_namespace(namespace)
+ ),
+ params={"details": "true"},
Review Comment:
[Nit] `list_metric_infos` repeats the whole request-plumbing block of
`list_metrics` (lines 92-99): the namespace check, the
`_format_metric_request_path(self._get_semantic_model_full_namespace(namespace))`
call and the error handler, with only `details` and the response class
differing. A small private helper taking `details` and returning `resp` would
keep the two modes from drifting if the path, the parameter name or the error
handler ever changes. Purely optional -- the current form is correct.
Verified by: comparing lines 91-107 with 109-125 in the checked-out file;
the only differences are the `details` value and the response type.
--
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]
