Re: [PR] [#12613] feat(client-python): Add Semantic Model DTOs and serialization [gravitino]
mchades merged PR #12859: URL: https://github.com/apache/gravitino/pull/12859 -- 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]
Re: [PR] [#12613] feat(client-python): Add Semantic Model DTOs and serialization [gravitino]
laserninja commented on code in PR #12859: URL: https://github.com/apache/gravitino/pull/12859#discussion_r4148256408 ## clients/client-python/gravitino/dto/semantic/json_serdes/semantic_json_mixin.py: ## @@ -0,0 +1,54 @@ +# 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. + +"""Lossless JSON text conversion for Semantic Model DTOs.""" + +import json +from decimal import Decimal + +import simplejson +from dataclasses_json import DataClassJsonMixin + + +class SemanticJsonMixin(DataClassJsonMixin): +"""JSON text conversion preserving numbers in semantic AI-context properties. + +Dict conversion leaves Decimal values intact. The standard dataclasses-json +encoder turns them into strings, so use a decimal-aware encoder at the JSON +boundary. Decode fractional numbers as Decimal before DTO conversion to avoid +rounding nested additional properties through binary floats. +""" + +def to_json(self, **kwargs) -> str: +"""Serialize to JSON, emitting Decimal values as exact JSON numbers. + +Keyword arguments are forwarded to simplejson.dumps, including formatting +options such as indent and sort_keys. +""" +return simplejson.dumps(self.to_dict(), use_decimal=True, **kwargs) Review Comment: Fixed in def1a9be0. Before JSON encoding, whole-number Decimals with exponent zero receive a fractional zero, so they serialize as decimal tokens and restore as Decimal. This is recursive and does not mutate stored values or depend on decimal-context precision. The regression covers 1, 0, -1, negative zero, large values, nested properties, and definition/model round trips, including equality, hashes, and dictionary lookups. Integer and boolean types remain unchanged. All 1,163 unit tests pass; Black and Pylint pass. -- 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]
Re: [PR] [#12613] feat(client-python): Add Semantic Model DTOs and serialization [gravitino]
mchades commented on code in PR #12859:
URL: https://github.com/apache/gravitino/pull/12859#discussion_r4141424907
##
clients/client-python/gravitino/dto/semantic/json_serdes/semantic_json_mixin.py:
##
@@ -0,0 +1,54 @@
+# 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.
+
+"""Lossless JSON text conversion for Semantic Model DTOs."""
+
+import json
+from decimal import Decimal
+
+import simplejson
+from dataclasses_json import DataClassJsonMixin
+
+
+class SemanticJsonMixin(DataClassJsonMixin):
+"""JSON text conversion preserving numbers in semantic AI-context
properties.
+
+Dict conversion leaves Decimal values intact. The standard dataclasses-json
+encoder turns them into strings, so use a decimal-aware encoder at the JSON
+boundary. Decode fractional numbers as Decimal before DTO conversion to
avoid
+rounding nested additional properties through binary floats.
+"""
+
+def to_json(self, **kwargs) -> str:
+"""Serialize to JSON, emitting Decimal values as exact JSON numbers.
+
+Keyword arguments are forwarded to simplejson.dumps, including
formatting
+options such as indent and sort_keys.
+"""
+return simplejson.dumps(self.to_dict(), use_decimal=True, **kwargs)
Review Comment:
[P2] Preserve decimal types for whole-number values
`simplejson` emits `Decimal("1")` (also `Decimal("0")` and `Decimal("-1")`)
as an integer JSON token. `from_json()` then restores it as `int`, since
`parse_float=Decimal` does not apply to integer tokens. The API distinguishes
integers from decimals, so a valid definition no longer equals its
round-tripped value and dictionary lookup by that definition fails. Please
retain a decimal/exponent token for these values and add a round-trip
regression, including nested additional properties.
--
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]
Re: [PR] [#12613] feat(client-python): Add Semantic Model DTOs and serialization [gravitino]
laserninja commented on code in PR #12859:
URL: https://github.com/apache/gravitino/pull/12859#discussion_r4135741743
##
clients/client-python/gravitino/dto/semantic/ai_context_object_dto.py:
##
@@ -0,0 +1,109 @@
+# 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
+from typing import Any, Optional
+
+from gravitino.api.semantic.ai_context_object import AIContextObject
+
+
+class AIContextObjectDTO:
+"""Represents a structured AI context DTO.
+
+Unknown properties are carried in `additional_properties` and are flattened
+next to the standard properties on the wire.
+"""
+
+def __init__(
+self,
+instructions: Optional[str] = None,
+synonyms: Optional[list[str]] = None,
+examples: Optional[list[str]] = None,
+additional_properties: Optional[dict[str, Any]] = None,
+):
+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 = (
+{}
+if additional_properties is None
+else copy.deepcopy(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 properties, empty if none are set."""
+return copy.deepcopy(self._additional_properties)
+
+@staticmethod
+def from_ai_context_object(
+ai_context_object: AIContextObject,
+) -> "AIContextObjectDTO":
+"""Convert a structured AI context to its DTO."""
+return AIContextObjectDTO(
+instructions=ai_context_object.instructions(),
+synonyms=ai_context_object.synonyms(),
+examples=ai_context_object.examples(),
+additional_properties=ai_context_object.additional_properties(),
+)
+
+def to_ai_context_object(self) -> AIContextObject:
+"""Convert this DTO to a structured AI context."""
+return AIContextObject(
+instructions=self._instructions,
+synonyms=self._synonyms,
+examples=self._examples,
+additional_properties=self._additional_properties,
+)
+
+def __eq__(self, other: object) -> bool:
+if not isinstance(other, AIContextObjectDTO):
+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 bbd8c09bc. AIContextObjectDTO equality and hashing now delegate to
the API object. Nested-value regressions verify boolean/integer distinctions,
float/Decimal equivalence, hash-based lookups, and propagation through
definition/model DTO equality. The full Python suite passes: 1,162 tests; Black
and Pylint also pass.
--
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]
Re: [PR] [#12613] feat(client-python): Add Semantic Model DTOs and serialization [gravitino]
laserninja commented on code in PR #12859:
URL: https://github.com/apache/gravitino/pull/12859#discussion_r4135739233
##
clients/client-python/gravitino/dto/semantic/semantic_model_definition_dto.py:
##
@@ -0,0 +1,121 @@
+# 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 dataclasses import dataclass, field
+from typing import Optional
+
+from dataclasses_json import DataClassJsonMixin, config
+
+from gravitino.api.semantic.semantic_model_definition import
SemanticModelDefinition
+from gravitino.dto.semantic.ai_context_dto import AIContextDTO
+from gravitino.dto.semantic.custom_extension_dto import CustomExtensionDTO
+from gravitino.dto.semantic.dataset_dto import DatasetDTO
+from gravitino.dto.semantic.json_serdes.ai_context_serdes import
AIContextSerdes
+from gravitino.dto.semantic.metric_dto import MetricDTO
+from gravitino.dto.semantic.relationship_dto import RelationshipDTO
+from gravitino.dto.semantic.semantic_dto_utils import convert_list, is_none
+
+
+@dataclass
+class SemanticModelDefinitionDTO(DataClassJsonMixin):
+"""Represents a Semantic Model definition DTO."""
+
+_ai_context: Optional[AIContextDTO] = field(
+default=None,
+metadata=config(
+field_name="ai_context",
Review Comment:
Fixed in bbd8c09bc. All eight field mappings now use the Java camelCase wire
names. Added an explicit Java-contract JSON regression that deserializes a
complete definition into the API and checks the outgoing JSON, including
relationships and optional fields.
##
clients/client-python/gravitino/dto/semantic/json_serdes/ai_context_serdes.py:
##
@@ -0,0 +1,104 @@
+# 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 Any, Optional, Union
+
+from gravitino.dto.semantic.ai_context_dto import AIContextDTO
+from gravitino.dto.semantic.ai_context_object_dto import AIContextObjectDTO
+from gravitino.utils.precondition import Precondition
+
+_INSTRUCTIONS = "instructions"
+_SYNONYMS = "synonyms"
+_EXAMPLES = "examples"
+
+
+class AIContextSerdes:
+"""Serdes for AI context DTOs.
+
+An AI context is either a bare JSON string or a JSON object whose unknown
+properties are retained losslessly.
+"""
+
+@staticmethod
+def serialize(
+value: Optional[AIContextDTO],
+) -> Optional[Union[str, dict[str, Any]]]:
+"""Encode an AI context DTO to a string or a dictionary."""
+if value is None:
+return None
+if value.text() is not None:
+return value.text()
+return _serialize_object(value.object())
+
+@staticmethod
+def deserialize(
+value: Optional[Union[str, dict[str, Any]]],
+) -> Optional[AIContextDTO]:
+"""Decode an AI context DTO from a string or a dictionary."""
+if value is None:
+return None
+if isinstance(value, str):
+return AIContextDTO(text=value)
+Precondition.check_argument(
+isinstance(value, dict), "AI context must be a string or object"
+)
+return AIContextDTO(obj=_deserialize_object(value))
+
+
+def _serialize_object(value: AIContextObjectDTO) -> dict[str, Any]:
+serialized: dict[str, Any] = {}
+if value.instructions() is not None:
+serialized[_INSTRUCTIONS] = value.instructions()
+if value.synonyms() is not None:
+serialized[_SYNONYMS] = value.synonyms()
+if value.examples() is not None:
+serialized[_EXAMPLES]
Re: [PR] [#12613] feat(client-python): Add Semantic Model DTOs and serialization [gravitino]
mchades commented on code in PR #12859: URL: https://github.com/apache/gravitino/pull/12859#discussion_r4130718064 ## clients/client-python/gravitino/dto/semantic/semantic_model_definition_dto.py: ## @@ -0,0 +1,121 @@ +# 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 dataclasses import dataclass, field +from typing import Optional + +from dataclasses_json import DataClassJsonMixin, config + +from gravitino.api.semantic.semantic_model_definition import SemanticModelDefinition +from gravitino.dto.semantic.ai_context_dto import AIContextDTO +from gravitino.dto.semantic.custom_extension_dto import CustomExtensionDTO +from gravitino.dto.semantic.dataset_dto import DatasetDTO +from gravitino.dto.semantic.json_serdes.ai_context_serdes import AIContextSerdes +from gravitino.dto.semantic.metric_dto import MetricDTO +from gravitino.dto.semantic.relationship_dto import RelationshipDTO +from gravitino.dto.semantic.semantic_dto_utils import convert_list, is_none + + +@dataclass +class SemanticModelDefinitionDTO(DataClassJsonMixin): +"""Represents a Semantic Model definition DTO.""" + +_ai_context: Optional[AIContextDTO] = field( +default=None, +metadata=config( +field_name="ai_context", Review Comment: [P1] Match the current Java DTO wire names The Java DTOs on this PR's base use `aiContext`, `customExtensions`, `primaryKey`, `uniqueKeys`, `isTime`, `vendorName`, `fromColumns` and `toColumns`. These Python DTOs use snake_case instead. Deserializing Java JSON silently drops the optional fields; a definition containing a relationship fails in `to_definition()` with `fromColumns must not be null or empty`. Please align all field mappings with the Java DTOs and test against their JSON fixtures; Python-only round trips currently hide this incompatibility. ## clients/client-python/gravitino/dto/semantic/json_serdes/ai_context_serdes.py: ## @@ -0,0 +1,104 @@ +# 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 Any, Optional, Union + +from gravitino.dto.semantic.ai_context_dto import AIContextDTO +from gravitino.dto.semantic.ai_context_object_dto import AIContextObjectDTO +from gravitino.utils.precondition import Precondition + +_INSTRUCTIONS = "instructions" +_SYNONYMS = "synonyms" +_EXAMPLES = "examples" + + +class AIContextSerdes: +"""Serdes for AI context DTOs. + +An AI context is either a bare JSON string or a JSON object whose unknown +properties are retained losslessly. +""" + +@staticmethod +def serialize( +value: Optional[AIContextDTO], +) -> Optional[Union[str, dict[str, Any]]]: +"""Encode an AI context DTO to a string or a dictionary.""" +if value is None: +return None +if value.text() is not None: +return value.text() +return _serialize_object(value.object()) + +@staticmethod +def deserialize( +value: Optional[Union[str, dict[str, Any]]], +) -> Optional[AIContextDTO]: +"""Decode an AI context DTO from a string or a dictionary.""" +if value is None: +return None +if isinstance(value, str): +return AIContextDTO(text=value) +Precondition.check_argument( +isinstance(value, dict), "AI context must be a string or object" +) +return AIContextDTO(obj=_deserialize_object(value)) + + +def _serialize_object(value: AIContextObjectDTO) ->
Re: [PR] [#12613] feat(client-python): Add Semantic Model DTOs and serialization [gravitino]
laserninja commented on PR #12859: URL: https://github.com/apache/gravitino/pull/12859#issuecomment-5875870617 Rebased onto the latest main and resolved the conflicts. Updated the PR description to reflect that #12858 is merged and this PR now contains only the DTO and serialization changes. -- 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]
Re: [PR] [#12613] feat(client-python): Add Semantic Model DTOs and serialization [gravitino]
mchades commented on PR #12859: URL: https://github.com/apache/gravitino/pull/12859#issuecomment-5871349201 please resolve the conflicts and update the PR description -- 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]
Re: [PR] [#12613] feat(client-python): Add Semantic Model DTOs and serialization [gravitino]
github-actions[bot] commented on PR #12859: URL: https://github.com/apache/gravitino/pull/12859#issuecomment-5516505249 ### Code Coverage Report |Overall Project|68.87%|:green_circle:| |:-|:-|:-:| |Files changed|No Java source files changed|-| |Module|Coverage|| |:-|:-|:-:| |aliyun|19.74%|:red_circle:| |api|51.62%|:green_circle:| |authorization-common|85.96%|:green_circle:| |authorization-ranger|4.38%|:red_circle:| |aws|53.54%|:green_circle:| |azure|32.1%|:red_circle:| |catalog-common|19.1%|:red_circle:| |catalog-fileset|80.3%|:green_circle:| |catalog-glue|69.24%|:green_circle:| |catalog-hive|82.96%|:green_circle:| |catalog-jdbc-common|45.69%|:green_circle:| |catalog-jdbc-doris|82.69%|:green_circle:| |catalog-jdbc-mysql|79.33%|:green_circle:| |catalog-jdbc-postgresql|83.39%|:green_circle:| |catalog-jdbc-starrocks|79.16%|:green_circle:| |catalog-kafka|76.99%|:green_circle:| |catalog-lakehouse-generic|60.55%|:green_circle:| |catalog-lakehouse-hudi|79.1%|:green_circle:| |catalog-lakehouse-iceberg|85.86%|:green_circle:| |catalog-lakehouse-paimon|84.26%|:green_circle:| |catalog-model|77.99%|:green_circle:| |cli|44.51%|:green_circle:| |client-java|77.37%|:green_circle:| |common|56.34%|:green_circle:| |core|83.86%|:green_circle:| |filesystem-hadoop3|76.45%|:green_circle:| |flink|0.0%|:red_circle:| |flink-common|52.1%|:green_circle:| |flink-runtime|0.0%|:red_circle:| |gcp|32.2%|:red_circle:| |hadoop-auth|68.0%|:green_circle:| |hadoop-common|17.84%|:red_circle:| |hive-metastore-common|53.4%|:green_circle:| |iceberg-aliyun-bundle|0.0%|:red_circle:| |iceberg-common|64.75%|:green_circle:| |iceberg-rest-server|75.96%|:green_circle:| |idp-basic|86.42%|:green_circle:| |integration-test-common|0.0%|:red_circle:| |jobs|62.92%|:green_circle:| |lance-common|32.52%|:red_circle:| |lance-rest-server|65.69%|:green_circle:| |lineage|53.02%|:green_circle:| |optimizer|83.17%|:green_circle:| |optimizer-api|21.95%|:red_circle:| |server|88.47%|:green_circle:| |server-common|80.67%|:green_circle:| |spark|28.57%|:red_circle:| |spark-common|48.92%|:green_circle:| |tencent|81.78%|:green_circle:| |trino-connector|51.26%|:green_circle:| -- 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]
