Copilot commented on code in PR #12858: URL: https://github.com/apache/gravitino/pull/12858#discussion_r4083862214
########## clients/client-python/gravitino/api/semantic/semantic_model_change.py: ########## @@ -0,0 +1,196 @@ +# 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 abc import ABC +from dataclasses import dataclass +from typing import Optional + +from gravitino.api.semantic.semantic_model_definition import SemanticModelDefinition +from gravitino.utils.precondition import Precondition + + +class SemanticModelChange(ABC): + """Defines changes that can be applied to a Semantic Model. + + Owner, tag, and policy changes use their existing governance stores and are + outside this contract. + """ + + @staticmethod + def rename(new_name: str) -> "RenameSemanticModel": + """Create a change for renaming a Semantic Model.""" + return RenameSemanticModel(new_name) + + @staticmethod + def update_comment(new_comment: Optional[str]) -> "UpdateComment": + """Create a change for updating or clearing a Semantic Model comment.""" + return UpdateComment(new_comment) + + @staticmethod + def set_property(property_name: str, value: str) -> "SetProperty": + """Create a change for setting a Semantic Model property.""" + return SetProperty(property_name, value) + + @staticmethod + def remove_property(property_name: str) -> "RemoveProperty": + """Create a change for removing a Semantic Model property.""" + return RemoveProperty(property_name) + + @staticmethod + def replace_definition( + definition: SemanticModelDefinition, + ) -> "ReplaceDefinition": + """Create a change for replacing the complete Semantic Model definition.""" + return ReplaceDefinition(definition) + + +@dataclass(frozen=True) +class RenameSemanticModel(SemanticModelChange): + """A SemanticModelChange to rename a Semantic Model.""" + + _new_name: str + + def __post_init__(self): + Precondition.check_string_not_empty( + self._new_name, "New name must not be null or blank" + ) + + def new_name(self) -> str: + """Returns the new Semantic Model name.""" + return self._new_name + + def __eq__(self, other: object) -> bool: + if not isinstance(other, RenameSemanticModel): + return False + return self._new_name == other.new_name() + + def __hash__(self) -> int: + return hash(self._new_name) + + def __str__(self) -> str: + return f"RENAMESEMANTICMODEL {self._new_name}" + + +@dataclass(frozen=True) +class UpdateComment(SemanticModelChange): + """A SemanticModelChange to update a Semantic Model comment.""" + + _new_comment: Optional[str] + + def new_comment(self) -> Optional[str]: + """Returns the new comment, `None` clears the current comment.""" + return self._new_comment + + def __eq__(self, other: object) -> bool: + if not isinstance(other, UpdateComment): + return False + return self._new_comment == other.new_comment() + + def __hash__(self) -> int: + return hash(self._new_comment) + + def __str__(self) -> str: + return f"UPDATECOMMENT {self._new_comment}" Review Comment: `UpdateComment.__str__` will render a clear operation as `UPDATECOMMENT None` when `_new_comment` is `None`. This is potentially misleading for logs/debugging and may be problematic if `__str__` is ever used for wire/command serialization. Consider formatting `None` explicitly (e.g., omitting the value or using a stable sentinel like `null`) and add a unit test asserting the intended string representation for the clear-comment case. ########## clients/client-python/gravitino/api/semantic/ai_context_object.py: ########## @@ -0,0 +1,189 @@ +# 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. + +import copy +import math +from typing import Any, Final, Optional + +from gravitino.api.semantic.semantic_utils import check_no_none_elements +from gravitino.exceptions.base import IllegalArgumentException +from gravitino.utils.precondition import Precondition + +MAX_ADDITIONAL_PROPERTY_NESTING_DEPTH: Final[int] = 100 + +_STANDARD_PROPERTIES: Final[frozenset] = frozenset( + {"instructions", "synonyms", "examples"} +) + + +class AIContextObject: + """The structured form of AI context attached to a Semantic Model member. + + Unknown JSON-compatible properties are exposed through + :meth:`additional_properties` and are retained losslessly. + """ + + def __init__( + self, + instructions: Optional[str] = None, + synonyms: Optional[list[str]] = None, + examples: Optional[list[str]] = None, + additional_properties: Optional[dict[str, Any]] = None, + ): + check_no_none_elements("synonyms", synonyms) + check_no_none_elements("examples", examples) + + self._instructions = instructions + self._synonyms = None if synonyms is None else list(synonyms) + self._examples = None if examples is None else list(examples) + self._additional_properties = _normalize_additional_properties( + additional_properties + ) + + def instructions(self) -> Optional[str]: + """Returns the free-form instructions, or `None` if it is not set.""" + return self._instructions + + def synonyms(self) -> Optional[list[str]]: + """Returns the synonyms, or `None` if they are not set.""" + return None if self._synonyms is None else list(self._synonyms) + + def examples(self) -> Optional[list[str]]: + """Returns the examples, or `None` if they are not set.""" + return None if self._examples is None else list(self._examples) + + def additional_properties(self) -> dict[str, Any]: + """Returns the additional JSON-compatible properties, empty if none are set.""" + return copy.deepcopy(self._additional_properties) + + def __eq__(self, other: object) -> bool: + if not isinstance(other, AIContextObject): + return False + return ( + self._instructions == other.instructions() + and self._synonyms == other.synonyms() + and self._examples == other.examples() + and self._additional_properties == other.additional_properties() + ) + + def __hash__(self) -> int: + return hash( + ( + self._instructions, + None if self._synonyms is None else tuple(self._synonyms), + None if self._examples is None else tuple(self._examples), + tuple(sorted(self._additional_properties)), + ) + ) Review Comment: `AIContextObject.__hash__` only hashes the *keys* of `additional_properties` (line 89) and ignores the values. While hash collisions are allowed, this can create very high collision rates when values differ, hurting performance in sets/dicts and making hashing less representative of equality. Recommend hashing a canonicalized representation that includes values (e.g., recursively freezing to tuples, or hashing a canonical JSON serialization after normalization) so that objects that differ in additional-property values are less likely to collide. ########## clients/client-python/gravitino/api/semantic/semantic_utils.py: ########## @@ -0,0 +1,59 @@ +# 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. + +"""Shared validation helpers for the Semantic Model value types.""" + +from typing import Optional, Sequence + +from gravitino.utils.precondition import Precondition + + +def check_no_none_elements(name: str, values: Optional[Sequence]) -> None: + """Check that an optional sequence does not contain `None` elements. + + Args: + name (str): The name reported in the error message. + values (Sequence, optional): The sequence to check, `None` is allowed. + + Raises: + IllegalArgumentException: If any element is `None`. + """ + if values is None: + return + for index, value in enumerate(values): + Precondition.check_argument( + value is not None, f"{name}[{index}] must not be null" + ) + + +def check_non_empty_string_elements(name: str, values: Optional[Sequence[str]]) -> None: + """Check that an optional sequence only contains non-empty strings. + + Args: + name (str): The name reported in the error message. + values (Sequence[str], optional): The sequence to check, `None` is allowed. + + Raises: + IllegalArgumentException: If any element is `None` or empty. + """ + if values is None: + return + for index, value in enumerate(values): + Precondition.check_argument( + value is not None and value != "", Review Comment: `check_non_empty_string_elements` claims to validate that elements are non-empty strings, but it currently does not enforce `isinstance(value, str)`. This will accept non-string values (e.g., integers) and may lead to inconsistent behavior later (serialization, equality, error reporting). Consider updating the predicate to require `isinstance(value, str)` (and the existing non-empty check), and add a unit test that demonstrates the expected failure for non-string elements. ########## clients/client-python/gravitino/api/catalog.py: ########## @@ -170,6 +170,20 @@ def as_view_catalog(self) -> "ViewCatalog": # noqa: F821 """ raise UnsupportedOperationException("Catalog does not support view operations") + def as_semantic_model_catalog(self) -> "SemanticModelCatalog": # noqa: F821 + """ + Raises: + UnsupportedOperationException if the catalog does not support Semantic + Model operations. + + Returns: + the {@link SemanticModelCatalog} if the catalog supports Semantic Model + operations. + """ Review Comment: The docstring uses JavaDoc-style `{@link ...}` which isn’t standard in Python doc tooling and may render poorly in generated docs. Consider switching to the doc style used elsewhere in this project (e.g., Sphinx-style `:class:` reference or simple backticks) for consistency and better documentation generation. -- 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]
