tengqm commented on code in PR #5646: URL: https://github.com/apache/gravitino/pull/5646#discussion_r1868687438
########## clients/client-python/gravitino/api/expressions/function_expression.py: ########## @@ -0,0 +1,95 @@ +# 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 __future__ import annotations +from abc import abstractmethod +from typing import List + +from gravitino.api.expressions.expression import Expression + + +class FunctionExpression(Expression): + """ + The interface of a function expression. A function expression is an expression that takes a + function name and a list of arguments. + """ + + @staticmethod + def of(function_name: str, *arguments: Expression) -> FuncExpressionImpl: + """ + Creates a new FunctionExpression with the given function name. + If no arguments are provided, it uses an empty expression. + + :param function_name: The name of the function. + :param arguments: The arguments to the function (optional). + :return: The created FunctionExpression. + """ + arguments = list(arguments) if arguments else Expression.EMPTY_EXPRESSION + return FuncExpressionImpl(function_name, arguments) + + @abstractmethod + def function_name(self) -> str: + """Returns the function name.""" + pass Review Comment: As noted previously, please reconsider all these `pass` cases ... do we really want to silently succeed here? ########## clients/client-python/gravitino/api/types/types.py: ########## @@ -123,7 +123,7 @@ def get(cls) -> "ShortType": return cls(True) @classmethod - def unsigned(cls): + def unsigned(cls) -> "ShortType": Review Comment: Can you help share with me what this syntax is? I mean ... why the return type is quoted? ########## clients/client-python/gravitino/api/expressions/function_expression.py: ########## @@ -0,0 +1,95 @@ +# 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 __future__ import annotations +from abc import abstractmethod +from typing import List + +from gravitino.api.expressions.expression import Expression + + +class FunctionExpression(Expression): + """ + The interface of a function expression. A function expression is an expression that takes a + function name and a list of arguments. + """ + + @staticmethod + def of(function_name: str, *arguments: Expression) -> FuncExpressionImpl: + """ + Creates a new FunctionExpression with the given function name. + If no arguments are provided, it uses an empty expression. + + :param function_name: The name of the function. + :param arguments: The arguments to the function (optional). + :return: The created FunctionExpression. + """ + arguments = list(arguments) if arguments else Expression.EMPTY_EXPRESSION + return FuncExpressionImpl(function_name, arguments) + + @abstractmethod + def function_name(self) -> str: + """Returns the function name.""" + pass + + @abstractmethod + def arguments(self) -> List[Expression]: + """Returns the arguments passed to the function.""" + pass + + def children(self) -> List[Expression]: + """Returns the arguments as children.""" + return self.arguments() + + +class FuncExpressionImpl(FunctionExpression): + """ + A concrete implementation of the FunctionExpression interface. + """ + + _function_name: str Review Comment: for a member that is well scoped, I don't think we need a sophisticated name. I'd suggest we use `_name` as the property name rather than `_function_name`. ########## clients/client-python/gravitino/api/expressions/literals/literals.py: ########## @@ -0,0 +1,143 @@ +# 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 decimal +from typing import TypeVar +from datetime import date, time, datetime + +from gravitino.api.expressions.literals.literal import Literal +from gravitino.api.types.type import Type +from gravitino.api.types.types import Types + +T = TypeVar("T") + + +class LiteralImpl(Literal[T]): + """Creates a literal with the given type value.""" + + _value: T + _data_type: Type + + def __init__( + self, + value: T, + data_type: Type, + ): Review Comment: ```suggestion def __init__(self, value: T, data_type: Type): ``` ########## clients/client-python/gravitino/api/expressions/named_reference.py: ########## @@ -0,0 +1,76 @@ +# 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 __future__ import annotations +from typing import List +from gravitino.api.expressions.expression import Expression + + +class NamedReference(Expression): + """ + Represents a field or column reference in the public logical expression API. + """ + + @staticmethod + def field(field_name: List[str]) -> FieldReference: + """Returns a FieldReference for the given field name(s).""" + return FieldReference(field_name) + + @staticmethod + def field_from_column(column_name: str) -> FieldReference: + """Returns a FieldReference for the given column name.""" + return FieldReference([column_name]) + + def field_name(self) -> List[str]: + """ + Returns the referenced field name as a list of string parts. + Must be implemented by subclasses. + """ + raise NotImplementedError("Subclasses must implement this method.") + + def children(self) -> List[Expression]: + """Named references do not have children.""" + return Expression.EMPTY_EXPRESSION + + def references(self) -> List[NamedReference]: + """Named references reference themselves.""" + return [self] + + +class FieldReference(NamedReference): + """ + A NamedReference that references a field or column. + """ + + def __init__(self, field_name: List[str]): + super().__init__() + self._field_name = field_name + + def field_name(self) -> List[str]: Review Comment: Not resolved? ########## clients/client-python/gravitino/api/expressions/function_expression.py: ########## @@ -0,0 +1,95 @@ +# 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 __future__ import annotations +from abc import abstractmethod +from typing import List + +from gravitino.api.expressions.expression import Expression + + +class FunctionExpression(Expression): + """ + The interface of a function expression. A function expression is an expression that takes a + function name and a list of arguments. + """ + + @staticmethod + def of(function_name: str, *arguments: Expression) -> FuncExpressionImpl: + """ + Creates a new FunctionExpression with the given function name. + If no arguments are provided, it uses an empty expression. + + :param function_name: The name of the function. + :param arguments: The arguments to the function (optional). + :return: The created FunctionExpression. + """ + arguments = list(arguments) if arguments else Expression.EMPTY_EXPRESSION + return FuncExpressionImpl(function_name, arguments) + + @abstractmethod + def function_name(self) -> str: + """Returns the function name.""" + pass + + @abstractmethod + def arguments(self) -> List[Expression]: + """Returns the arguments passed to the function.""" + pass + + def children(self) -> List[Expression]: + """Returns the arguments as children.""" + return self.arguments() + + +class FuncExpressionImpl(FunctionExpression): + """ + A concrete implementation of the FunctionExpression interface. + """ + + _function_name: str Review Comment: similarly, given an instance `impl` of class `FuncExpressionImpl`, the following calls are still pretty clear without the complex names: ``` def __init__(self, name: str, arguments: List[Expression]): .... def name(self) -> str: ... ``` ########## clients/client-python/gravitino/api/expressions/expression.py: ########## @@ -0,0 +1,51 @@ +# 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 __future__ import annotations +from abc import ABC, abstractmethod +from typing import List, Set, TYPE_CHECKING Review Comment: I'm new to this `typing` thing. But according to Python docs, these alias are all deprecated in Python 3.9. cf. https://docs.python.org/3/library/typing.html#deprecated-aliases It might not be a good idea to use them, IIUC. -- 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]
