Copilot commented on code in PR #14147:
URL: https://github.com/apache/gravitino/pull/14147#discussion_r4401065221
##########
clients/client-python/gravitino/api/semantic/semantic_model_catalog.py:
##########
@@ -55,6 +56,44 @@ 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: their namespace contains the
+ schema and Semantic Model names, followed by the exact metric name.
+ Results are ordered by model identifier and then case-sensitive metric
name.
Review Comment:
The docstring suggests the metric name is part of the identifier namespace
(\"followed by the exact metric name\"), but `NameIdentifier` splits
`namespace` vs `name`. Given the implementation/tests, the namespace is
(schema, model) and the identifier `name` is the metric name. Updating this
wording will prevent API consumers from constructing identifiers incorrectly.
##########
clients/client-python/gravitino/api/semantic/semantic_model_metric.py:
##########
@@ -0,0 +1,56 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from gravitino.api.semantic.metric import Metric
+from gravitino.namespace import Namespace
+from gravitino.utils.precondition import Precondition
+
+
+class SemanticModelMetric:
+ """A discovered metric with its containing Semantic Model's namespace.
+
+ Catalog-scoped discovery returns a namespace containing the schema and
model
+ names. The metric's name remains part of its complete :class:`Metric`
value.
+ """
+
+ def __init__(self, namespace: Namespace, metric: Metric):
+ Precondition.check_argument(namespace is not None, "namespace must not
be null")
+ Precondition.check_argument(metric is not None, "metric must not be
null")
+ self._namespace = Namespace.of(*namespace.levels())
+ self._metric = metric
+
+ def namespace(self) -> Namespace:
+ """Return a copy of the namespace ending with the containing model's
name."""
+ return Namespace.of(*self._namespace.levels())
+
+ def metric(self) -> Metric:
+ """Return the complete metric definition."""
+ return self._metric
+
+ def __eq__(self, other: object) -> bool:
+ if not isinstance(other, SemanticModelMetric):
+ return False
+ return self._namespace == other.namespace() and self._metric ==
other.metric()
Review Comment:
`__eq__` currently calls `other.namespace()` which creates a copy each time,
adding avoidable allocations during comparisons (and makes equality depend on
accessor behavior). Since this is comparing two instances of the same class,
compare internal fields directly (e.g., `other._namespace` / `other._metric`)
to keep equality fast and self-contained.
##########
clients/client-python/gravitino/client/semantic_model_catalog_operations.py:
##########
@@ -291,6 +329,21 @@ def _format_semantic_model_request_path(namespace:
Namespace) -> str:
"/semantic-models"
)
+ @staticmethod
+ def _format_metric_request_path(namespace: Namespace) -> str:
+ return (
+ f"api/metalakes/{encode_string(namespace.level(0))}"
+ f"/catalogs/{encode_string(namespace.level(1))}"
+ f"/schemas/{encode_string(namespace.level(2))}"
+ "/semantic-model-metrics"
+ )
+
+ @staticmethod
+ def _relative_metric_namespace(namespace: Namespace) -> Namespace:
+ if namespace is None or namespace.length() != 4:
+ raise IllegalArgumentException("Metric namespace must have 4
levels")
Review Comment:
The exception message is a bit opaque when debugging server responses.
Consider including the actual level count (and/or the received namespace
levels) and the expected shape (metalake/catalog/schema/model), e.g.,
\"Expected metric namespace with 4 levels (metalake/catalog/schema/model), got
{namespace.length()}: {namespace.levels()}\".
--
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]
