[#5201] feat(client-python): Implement expressions in python client #5646

SophieTech88 · 2024-11-22T06:24:26Z

What changes were proposed in this pull request?

Implement expression from java, including:

Expression.java
FunctionExpression.java
NamedReference.java
UnparsedExpression.java
literals/

convert to python client, and add unit test for each class.

Why are the changes needed?

We need to support the expressions in python client

Fix: #5201

Does this PR introduce any user-facing change?

No

How was this patch tested?

Need to pass all unit tests.

clients/client-python/gravitino/api/expressions/named_reference.py

clients/client-python/gravitino/api/expressions/function_expression.py

jerryshao · 2024-11-25T03:27:22Z

@mchades @xunliu would you please also help to review this PR?

xunliu · 2024-11-25T11:13:05Z

hi @SophieTech88 I will help you improve this PR.
Please review my commit.

tengqm

I'd suggest we split this into a few smaller PRs.

tengqm · 2024-11-25T11:59:55Z

clients/client-python/gravitino/api/expressions/Literals/literal.py

+        pass
+
+    @abstractmethod
+    def data_type(self):


Sounds like that the above two methods are be implemented by subclasses anyway.
If that is true, I don't think we we do a pass here.
We may want to raise an exception instead.

Agree. Just updated the code to raise NotImplementedError() for those 2 functions.

clients/client-python/gravitino/api/expressions/Literals/literals.py

mchades

I'd suggest we split this into a few smaller PRs.

Agree with @tengqm, the current PR is too large to review.

As this PR is focused on expressions, I suggest moving distributions, sorts, and transforms to separate PRs. This will make it easier for us to review this PR. WDYT? @SophieTech88

SophieTech88 · 2024-11-26T14:50:35Z

I'd suggest we split this into a few smaller PRs.

Agree with @tengqm, the current PR is too large to review.

As this PR is focused on expressions, I suggest moving distributions, sorts, and transforms to separate PRs. This will make it easier for us to review this PR. WDYT? @SophieTech88

Of course. Just update the PR, removed distributions, sorts, and transforms .

mchades · 2024-11-26T15:16:17Z

clients/client-python/gravitino/api/expressions/literals/literals.py

+
+    _value: Union[int, float, str, datetime, time, date, bool, Decimal, None]
+    _data_type: (
+        str  # TODO: Need implement `api/src/main/java/org/apache/gravitino/rel/types`


you already implemented types clients/client-python/gravitino/api/types.py and type clients/client-python/gravitino/api/type.py

Oh. Yeah. Do I need to import and list all types in this _data_type?

I just update the _data_type in the new commit

mchades · 2024-11-27T02:01:16Z

clients/client-python/gravitino/api/expressions/literals/literals.py

+    """Creates a literal with the given type value."""
+
+    _value: Union[int, float, Decimal, str, date, time, datetime, bool, None]
+    _data_type: Union[


seems should be _data_type: Type? @xunliu cc

I improve this part codes
@SophieTech88 @noidname01 Please help me review, thanks.

The changes for the types things, LGTM. Many thanks!

noidname01 · 2024-12-02T10:15:22Z

clients/client-python/gravitino/api/expressions/expression.py

+class Expression(ABC):
+    """Base class of the public logical expression API."""
+
+    EMPTY_EXPRESSION: List[Expression] = []


This might cause unexpected behaviour if it will be modified at any moment (like append, delete, e.g.), because this list would share between classes, any modification would affect where it exists once and for all at the same time. Unless we're sure that the scenario above would not happen, I think it's better to use immutable object here or some other methods.

hi @noidname01
The EMPTY_EXPRESSION value does not fill in any data.
So, I think we didn't need to modify this code, what's do you think?

Sure, then overall LGTM!

mchades · 2024-12-02T12:10:18Z

clients/client-python/gravitino/api/types/decimal.py

this is unnecessary since #5354 (comment)

@mchades I removed Decimal type. Please help me review again, Thanks.

mchades

Overall LGTM. Do you want to have another look? @tengqm

jerryshao · 2024-12-03T09:05:44Z

LGTM.

xunliu · 2024-12-04T02:22:57Z

hi @tengqm
Do you want to have another look?

tengqm · 2024-12-04T03:56:15Z

clients/client-python/gravitino/api/expressions/function_expression.py

+    A concrete implementation of the FunctionExpression interface.
+    """
+
+    _function_name: str


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.

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: ...

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.

and

similarly, given an instance impl of class FuncExpressionImpl,
the following calls are still pretty clear without the complex names:

hi @tengqm
Will want to keep Python code consistent with Java code, https://github.com/apache/gravitino/blob/main/api/src/main/java/org/apache/gravitino/rel/expressions/FunctionExpression.java#L66

tengqm · 2024-12-04T04:03:44Z

clients/client-python/gravitino/api/expressions/function_expression.py

+    @abstractmethod
+    def function_name(self) -> str:
+        """Returns the function name."""
+        pass


As noted previously, please reconsider all these pass cases ...
do we really want to silently succeed here?

Yes, You are right, I will fix this problem.

tengqm · 2024-12-04T04:04:31Z

clients/client-python/gravitino/api/expressions/literals/literals.py

+    def __init__(
+        self,
+        value: T,
+        data_type: Type,
+    ):


Suggested change

def __init__(

self,

value: T,

data_type: Type,

):

def __init__(self, value: T, data_type: Type):

clients/client-python/gravitino/api/expressions/named_reference.py

tengqm · 2024-12-04T04:22:39Z

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":


Can you help share with me what this syntax is?
I mean ... why the return type is quoted?

class ShortType(IntegralType): _instance: "ShortType" = None _unsigned_instance: "ShortType" = None

The type annotation ShortType is quoted because _instance is an attribute of the class that references ShortType, and the class itself isn't fully defined at the time this attribute is declared.

The return type annotations (-> "ShortType") are also quoted for the same reason: ShortType isn't fully defined when these methods are declared.

If we remove the quote, there is a Pylance errors, and it won't pass the pylint check.

Em... I am not a big fan of passing lint checking simply for passing's purpose.
At the end of the day, we are the maintainers to maintain these code.
If lint rules are not appropriate in our context, we may want to disable that
particular rule. It is not an uncommon practice.

At the same time, if we are forced to "hack" the code to make the linter happy,
we may need to revise our design. Probably there are more straightforward ways
to achieve the same end.

tengqm · 2024-12-04T04:24:10Z

clients/client-python/gravitino/api/expressions/expression.py

+
+from __future__ import annotations
+from abc import ABC, abstractmethod
+from typing import List, Set, TYPE_CHECKING


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.

Yeah. List, Set are deprecated from typing in Python 3.9. We can use list and set directly.

SophieTech88 · 2024-12-04T06:40:30Z

The Integration Test failure is related to the List typing things in literal.py file.

xunliu · 2024-12-04T08:03:55Z

hi @tengqm We are improving PR base on your comments,
Please help me review it again, thanks!

tengqm · 2024-12-04T08:44:15Z

clients/client-python/gravitino/api/expressions/literals/literal.py

+# under the License.
+
+from abc import abstractmethod
+from typing import List, TypeVar, Generic


You may want to kill 'List' here.

tengqm · 2024-12-04T08:46:03Z

clients/client-python/gravitino/api/expressions/literals/literals.py

+        return f"LiteralImpl(value={self._value}, data_type={self._data_type})"
+
+
+class Literals:


Since we already have a 'Literal' class in another file,
defining a new class called 'Literals' may confuse other developers/users.

Further more, it doesn't make a lot senses to me to define a class as a wrapper
for so many class methods.
Python is not a pure object-oriented language, after all.

hi @tengqm I know what you mean.
We also refer to Iceberg's Python client code style.
https://github.com/apache/iceberg-python/blob/main/pyiceberg/table/name_mapping.py
At present, there are not enough experienced Python engineers in the community, which is the result of the best efforts of the community contributors.
We keep the python client code consistent with the java code to ensure that the functionality is consistent.
So I think we didn't modify these codes.

xunliu · 2024-12-04T09:46:54Z

When I use list to replace List.
The TestLiterals throw these errors.

The error TypeError: 'type' object is not subscriptable occurs because the list type is not subscriptable in Python versions earlier than 3.9. To fix this, you should use List from the typing module instead of list.

@tengqm @noidname01 Do you have any suggestions to fix this problem? I rollback this change??

xunliu · 2024-12-04T10:02:41Z

hi @tengqm
Gravitino Python client went to support from 3.8 to 3.11, So maybe we must use typing.List type to support low Python version.

tengqm · 2024-12-04T10:49:21Z

hi @tengqm Gravitino Python client went to support from 3.7 to 3.10, So maybe we must use typing.List type to support low Python version.

In that case, we have to stick to the old way.
3.7-3.10 is a large range anyway.

tengqm · 2024-12-04T11:05:53Z

Just check the github workflow for this project:

https://github.com/apache/gravitino/blob/main/.github/workflows/python-integration-test.yml#L76

Looks like we are testing against 3.8, 3.9, 3.10, 3.11.

This reverts commit d3dc38a.

xunliu · 2024-12-04T13:14:21Z

hi @tengqm :-)
I revert remove List commit, Please help me review it again, Thanks!

tengqm · 2024-12-04T15:04:44Z

lgtm

mchades

I am merging this into the main branch. Thank you all for your efforts on this PR!
@SophieTech88 @tengqm @noidname01 @xunliu

…5646) ### What changes were proposed in this pull request? Implement expression from java, including: - Expression.java - FunctionExpression.java - NamedReference.java - UnparsedExpression.java - literals/ convert to python client, and add unit test for each class. ### Why are the changes needed? We need to support the expressions in python client Fix: #5201 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Need to pass all unit tests. --------- Co-authored-by: Xun <[email protected]>

[5201]Implement expressions in python client

bade12b

SophieTech88 marked this pull request as draft November 22, 2024 06:24

tengqm reviewed Nov 22, 2024

View reviewed changes

SophieTech88 added 4 commits November 23, 2024 00:03

Update the script for named_reference and function_expression

3fc7400

Update the distribution.py

00d31e8

Update literals.py

c21bba5

Update sorts.py

361acbc

xunliu assigned SophieTech88 Nov 25, 2024

jerryshao requested review from xunliu and mchades November 25, 2024 03:26

SophieTech88 added 2 commits November 24, 2024 23:01

Update the transforms.py

07a8a87

Fix a f string bug for distributions.py

2bc8983

SophieTech88 marked this pull request as ready for review November 25, 2024 05:21

SophieTech88 and others added 2 commits November 24, 2024 23:26

Update the comment for Partition class and so on

4bac456

improve model structure

40a14ee

tengqm reviewed Nov 25, 2024

View reviewed changes

SophieTech88 added 2 commits November 25, 2024 20:19

Update the script and fix a name bug

2a25eb4

Update the Literal class to raise NotImplementedError

3bc79ca

xunliu changed the title ~~[5201]Implement expressions in python client~~ [5201] feat(client-python): Implement expressions in python client Nov 26, 2024

xunliu and others added 3 commits November 26, 2024 13:51

remove Literals folder

9fe8d6c

Add literals folder

cbb2a55

Update licenses headers and format

624929e

mchades reviewed Nov 26, 2024

View reviewed changes

Remove distributions, sorts, and transforms to separate PRs

c3d8271

mchades reviewed Nov 26, 2024

View reviewed changes

Update the data type in literals.py

6042765

mchades reviewed Nov 27, 2024

View reviewed changes

xunliu added 2 commits December 2, 2024 12:49

imporve type in the literal.py

45ae0c3

fix CI

88acd36

noidname01 suggested changes Dec 2, 2024

View reviewed changes

mchades reviewed Dec 2, 2024

View reviewed changes

xunliu added 2 commits December 3, 2024 14:48

delete Decimal

2c0e316

fix CI

8a28e7d

mchades previously approved these changes Dec 3, 2024

View reviewed changes

jerryshao changed the title ~~[5201] feat(client-python): Implement expressions in python client~~ [#5201] feat(client-python): Implement expressions in python client Dec 3, 2024

tengqm reviewed Dec 4, 2024

View reviewed changes

Update the typing for list and set

3750bbf

SophieTech88 dismissed mchades’s stale review via 3750bbf December 4, 2024 06:15

SophieTech88 and others added 2 commits December 4, 2024 00:41

Fix CI: import List in literal.py

82333a7

remove pass

89b56bf

tengqm reviewed Dec 4, 2024

View reviewed changes

remove List

d3dc38a

Revert "remove List"

c68c530

This reverts commit d3dc38a.

mchades approved these changes Dec 5, 2024

View reviewed changes

mchades merged commit 10fc5cd into apache:main Dec 5, 2024
23 checks passed

		return f"LiteralImpl(value={self._value}, data_type={self._data_type})"


		class Literals:

[#5201] feat(client-python): Implement expressions in python client #5646

[#5201] feat(client-python): Implement expressions in python client #5646

Conversation

SophieTech88 commented Nov 22, 2024 • edited by mchades Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

jerryshao commented Nov 25, 2024

xunliu commented Nov 25, 2024

tengqm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchades left a comment

Choose a reason for hiding this comment

SophieTech88 commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchades left a comment

Choose a reason for hiding this comment

jerryshao commented Dec 3, 2024

xunliu commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SophieTech88 commented Dec 4, 2024

xunliu commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

xunliu commented Dec 4, 2024

xunliu commented Dec 4, 2024 • edited Loading

tengqm commented Dec 4, 2024

tengqm commented Dec 4, 2024

xunliu commented Dec 4, 2024

tengqm commented Dec 4, 2024

mchades left a comment • edited Loading

Choose a reason for hiding this comment

SophieTech88 commented Nov 22, 2024 •

edited by mchades

Loading

xunliu Dec 4, 2024 •

edited

Loading

xunliu commented Dec 4, 2024 •

edited

Loading

mchades left a comment •

edited

Loading