laserninja commented on code in PR #12858: URL: https://github.com/apache/gravitino/pull/12858#discussion_r4084978108
########## 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: Fixed in a3ca236cd. Hashing now recursively includes both keys and values, with type tags shared by equality. Tests cover differing values, nested containers, and dictionary insertion-order independence. ########## 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: Fixed in a3ca236cd. Clearing a comment now renders as `UPDATECOMMENT null`, with a regression test for this case. ########## 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: Fixed in a3ca236cd. The predicate now requires a non-empty string. Regression tests reject non-string primary keys, unique keys, and both relationship column lists. ########## 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: Fixed in a3ca236cd. Replaced the JavaDoc link with a fully qualified Sphinx `:class:` reference. ########## clients/client-python/gravitino/api/semantic/dataset.py: ########## @@ -0,0 +1,165 @@ +# 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 typing import Optional + +from gravitino.api.semantic.ai_context import AIContext +from gravitino.api.semantic.custom_extension import CustomExtension +from gravitino.api.semantic.field import Field +from gravitino.api.semantic.semantic_utils import ( + check_no_none_elements, + check_non_empty_string_elements, +) +from gravitino.name_identifier import NameIdentifier +from gravitino.utils.precondition import Precondition + + +class Dataset: # pylint: disable=too-many-instance-attributes + """A source-backed dataset exposed by a Semantic Model. + + Dataset names are unique within a Semantic Model. The source is a three-part + `NameIdentifier` that must resolve to a table or a logical view in the same + metalake, inline query sources are not supported. + """ + + def __init__( + self, + name: str, + source: NameIdentifier, + primary_key: Optional[list[str]] = None, + unique_keys: Optional[list[list[str]]] = None, + description: Optional[str] = None, + ai_context: Optional[AIContext] = None, + fields: Optional[list[Field]] = None, + custom_extensions: Optional[list[CustomExtension]] = None, + ): + Precondition.check_argument( + name is not None and name != "", "name must not be null or empty" + ) + Precondition.check_argument(source is not None, "source must not be null") + check_non_empty_string_elements("primaryKey", primary_key) + _check_unique_keys(unique_keys) + check_no_none_elements("fields", fields) + check_no_none_elements("customExtensions", custom_extensions) + + self._name = name + self._source = source Review Comment: Fixed in a3ca236cd. Dataset now copies the identifier and namespace on construction and when returning the source. The regression test mutates both the caller's identifier and the returned identifier, verifying that definition equality, its hash, and dictionary lookup remain stable. ########## 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() Review Comment: Fixed in a3ca236cd. Equality and hashing now share a recursive, type-aware representation of normalized JSON values. Regression tests distinguish booleans from numbers at the top level and inside nested lists/maps, including definitions containing those contexts. ########## 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) Review Comment: Fixed in a3ca236cd. Both synonyms and examples now validate string elements before storing them. Tests reject dictionaries, lists, numbers, booleans, and None. Empty strings remain accepted under the existing contract. -- 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]
