tsungchih commented on code in PR #9870:
URL: https://github.com/apache/gravitino/pull/9870#discussion_r2776698282
##########
clients/client-python/gravitino/api/rel/types/json_serdes/_helper/serdes_utils.py:
##########
@@ -309,3 +322,202 @@ def read_external_type(cls, external_data: Dict[str,
Any]) -> Types.ExternalType
f"Cannot parse external type from missing catalogString:
{external_data}",
)
return Types.ExternalType.of(external_data[cls.CATALOG_STRING])
+
+ @classmethod
+ def column_default_value_encoder(cls, value: Expression) -> Union[str,
None]:
+ if value is None or value == Expression.EMPTY_EXPRESSION:
+ return None
+
+ return cls.write_function_arg(value)
+
+ @classmethod
+ def write_function_arg(cls, arg: FunctionArg) -> Any:
Review Comment:
I think this method has been implemented in
[`gravitino.dto.rel.expressions.json_serdes._helper.serdes_utils`](https://github.com/apache/gravitino/blob/40007c804c21572b9ec015e5e3c1b04665d4dcce/clients/client-python/gravitino/dto/rel/expressions/json_serdes/_helper/serdes_utils.py#L35).
##########
clients/client-python/gravitino/api/rel/types/json_serdes/_helper/serdes_utils.py:
##########
Review Comment:
The `SerdesUtils` in this module is a helper class aimed at
serializing/deserializing types in Gravitino expression system. I think we
should keep it focused and separate the Ser/Des for table index and column
position as two individual classes. For example, the [sort order
Ser/Des](https://github.com/apache/gravitino/blob/40007c804c21572b9ec015e5e3c1b04665d4dcce/clients/client-python/gravitino/dto/rel/json_serdes/sort_order_serdes.py)
and the [distribution
Ser/Des](https://github.com/apache/gravitino/blob/40007c804c21572b9ec015e5e3c1b04665d4dcce/clients/client-python/gravitino/dto/rel/json_serdes/distribution_serdes.py).
In doing so, I would recommend to put these two Ser/Des classes under
`gravitino.api.rel.json_serdes.xxxxxx_serdes`.
##########
clients/client-python/gravitino/api/rel/types/json_serdes/_helper/serdes_utils.py:
##########
@@ -309,3 +322,202 @@ def read_external_type(cls, external_data: Dict[str,
Any]) -> Types.ExternalType
f"Cannot parse external type from missing catalogString:
{external_data}",
)
return Types.ExternalType.of(external_data[cls.CATALOG_STRING])
+
+ @classmethod
+ def column_default_value_encoder(cls, value: Expression) -> Union[str,
None]:
+ if value is None or value == Expression.EMPTY_EXPRESSION:
+ return None
+
+ return cls.write_function_arg(value)
+
+ @classmethod
+ def write_function_arg(cls, arg: FunctionArg) -> Any:
+ if arg.arg_type() == FunctionArg.ArgType.LITERAL:
+ return {
+ cls.TYPE: arg.arg_type().value.lower(),
+ cls.DATA_TYPE: cls.write_data_type(arg.data_type()),
+ cls.LITERAL_VALUE: arg.value(),
+ }
+
+ if arg.arg_type() == FunctionArg.ArgType.FIELD:
+ return {
+ cls.TYPE: arg.arg_type().value.lower(),
+ cls.FIELD_NAME: arg.field_name(),
+ }
+
+ if arg.arg_type() == FunctionArg.ArgType.FUNCTION:
+ return {
+ cls.TYPE: arg.arg_type().value.lower(),
+ cls.FUNCTION_NAME: arg.function_name(),
+ cls.FUNCTION_ARGS: [
+ cls.write_function_arg(child) for child in arg.args()
+ ],
+ }
+
+ if arg.arg_type() == FunctionArg.ArgType.UNPARSED:
+ return {
+ cls.TYPE: arg.arg_type().value.lower(),
+ cls.UNPARSED_EXPRESSION: arg.unparsed_expression(),
+ }
+
+ raise ValueError(f"Unknown function argument type: {arg.arg_type()}")
+
+ @classmethod
+ def column_default_value_decoder(cls, value: dict) -> Expression:
+ if value is None:
+ return Expression.EMPTY_EXPRESSION
+
+ return cls.read_function_arg(value)
+
+ @classmethod
+ def read_function_arg(cls, value: dict[str, Any]) -> Expression:
Review Comment:
I think this method has been implemented in
[`gravitino.dto.rel.expressions.json_serdes._helper.serdes_utils`](https://github.com/apache/gravitino/blob/40007c804c21572b9ec015e5e3c1b04665d4dcce/clients/client-python/gravitino/dto/rel/expressions/json_serdes/_helper/serdes_utils.py#L65).
##########
clients/client-python/gravitino/api/rel/types/json_serdes/_helper/serdes_utils.py:
##########
@@ -309,3 +322,202 @@ def read_external_type(cls, external_data: Dict[str,
Any]) -> Types.ExternalType
f"Cannot parse external type from missing catalogString:
{external_data}",
)
return Types.ExternalType.of(external_data[cls.CATALOG_STRING])
+
+ @classmethod
+ def column_default_value_encoder(cls, value: Expression) -> Union[str,
None]:
+ if value is None or value == Expression.EMPTY_EXPRESSION:
+ return None
+
+ return cls.write_function_arg(value)
+
+ @classmethod
+ def write_function_arg(cls, arg: FunctionArg) -> Any:
+ if arg.arg_type() == FunctionArg.ArgType.LITERAL:
+ return {
+ cls.TYPE: arg.arg_type().value.lower(),
+ cls.DATA_TYPE: cls.write_data_type(arg.data_type()),
+ cls.LITERAL_VALUE: arg.value(),
+ }
+
+ if arg.arg_type() == FunctionArg.ArgType.FIELD:
+ return {
+ cls.TYPE: arg.arg_type().value.lower(),
+ cls.FIELD_NAME: arg.field_name(),
+ }
+
+ if arg.arg_type() == FunctionArg.ArgType.FUNCTION:
+ return {
+ cls.TYPE: arg.arg_type().value.lower(),
+ cls.FUNCTION_NAME: arg.function_name(),
+ cls.FUNCTION_ARGS: [
+ cls.write_function_arg(child) for child in arg.args()
+ ],
+ }
+
+ if arg.arg_type() == FunctionArg.ArgType.UNPARSED:
+ return {
+ cls.TYPE: arg.arg_type().value.lower(),
+ cls.UNPARSED_EXPRESSION: arg.unparsed_expression(),
+ }
+
+ raise ValueError(f"Unknown function argument type: {arg.arg_type()}")
+
+ @classmethod
+ def column_default_value_decoder(cls, value: dict) -> Expression:
Review Comment:
We have implemented the Ser/Des for column default value
[here](https://github.com/apache/gravitino/blob/40007c804c21572b9ec015e5e3c1b04665d4dcce/clients/client-python/gravitino/dto/rel/expressions/json_serdes/column_default_value_serdes.py#L28)
##########
clients/client-python/gravitino/api/rel/types/json_serdes/_helper/serdes_utils.py:
##########
@@ -309,3 +322,202 @@ def read_external_type(cls, external_data: Dict[str,
Any]) -> Types.ExternalType
f"Cannot parse external type from missing catalogString:
{external_data}",
)
return Types.ExternalType.of(external_data[cls.CATALOG_STRING])
+
+ @classmethod
+ def column_default_value_encoder(cls, value: Expression) -> Union[str,
None]:
Review Comment:
We have implemented the Ser/Des for column default value
[here](https://github.com/apache/gravitino/blob/40007c804c21572b9ec015e5e3c1b04665d4dcce/clients/client-python/gravitino/dto/rel/expressions/json_serdes/column_default_value_serdes.py#L28).
##########
clients/client-python/gravitino/dto/rel/indexes/index_dto.py:
##########
@@ -47,6 +49,10 @@ def __init__(
self._name = name
self._field_names = field_names
+ @staticmethod
+ def builder() -> IndexDTO.Builder:
Review Comment:
I'm curious why do we need a builder method. IMHO, it is possible to prevent
the caller from initiating an instance directly and use the builder method in
Java. However, we cannot implement it in Python that way so far as I know. The
user can still directly initiate an instance even we provide the builder
method. In this case, do we still need to provide the builder method? And why
do we need to provide the builder method?
--
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]
