[#5203] feat(client-python): porting partitions from java client

[unknowntpo]

What changes were proposed in this pull request?

Porting interface Partitions, interface IdentityPartition, interface ListPartition, interface RangePartition, and class Partitions from java to python.

Fix: #5203

Does this PR introduce any user-facing change?

Yes.

How was this patch tested?

Unit tests.

[xunliu]

I think we need to use clients/client-python/gravitino/api/expressions/literals/literal.py, maybe like this?

class IdentityPartition(Partition):

    @abstractmethod
    def field_names(self) -> List[List[str]]:
        pass

    @abstractmethod
    def values(self) -> List[Literal]:
        pass

[unknowntpo]

@xunliu thanks for you advice, but I have a little suggestion.

In IdentityPartition.java, the method signature is

public interface IdentityPartition extends Partition {

  /** @return The field names of the identity partition. */
  String[][] fieldNames();

  /**
   * @return The values of the identity partition. The values are in the same order as the field
   *     names.
   */
  Literal<?>[] values();
}

where values() returns array of Literal with wildcard type parameter.

so in Python, we need to define the method as

class IdentityPartition(Partition):
    ...
    @abstractmethod
    def values(self) -> List[Literal[Any]]:

[xunliu]

We can use Literal, This line code may be like this?

def lists(self) -> List[List[Literal]]:

[unknowntpo]

We can use Literal, This line code may be like this?

def lists(self) -> List[List[Literal]]:

We should defined it as

def lists(self) -> List[List[Literal[Any]]]:

[unknowntpo]

@xunliu please have a look.

[jerryshao]

@xunliu @mchades would you please help to review?

[xunliu]

I think need add

    if self is other:
        return True

in here?

[unknowntpo]

@xunliu We can not use is to compare objects, because it will be True only if they refer to the same object in memory.

To verify it, use this simple script:

class Hello: 
    def __init__(self, a: int, b: int) -> None:
        self.a = a
        self.b = b

def main():
    objA = Hello(3, 2)
    objB = Hello(3, 2)

    print(objA is objB) # will be False


if __name__ == "__main__":
    main()

The correct way is to check if they belong to the same class first, and check equality of each fields.
Just like what we did at catalog_change.py:

        def __eq__(self, other) -> bool:
            """Compares this SetProperty instance with another object for equality.
            Two instances are considered equal if they have the same property and value for the catalog.

            Args:
                other: The object to compare with this instance.

            Returns:
                true if the given object represents the same property setting; false otherwise.
            """
            if not isinstance(other, CatalogChange.SetProperty):
                return False
            return self.property() == other.property() and self.value() == other.value()

[xunliu]

I think need add

    if self is other:
        return True

in here?

[xunliu]

I think need add

    if self is other:
        return True

in here?

[antony0016]

Can here do early return here to reduce indent and follow how catalog_change.py did?

if not isinstance(other, RangePartitionImpl):
    return False
return (
    self._name == other._name
    and self._upper == other._upper
    and self._lower == other._lower
    and self._properties == other._properties
)

[antony0016]

Can here do early return here to reduce indent and follow how catalog_change.py did?

if not isinstance(other, ListPartitionImpl):
    return False
return (
    self._name == other._name
    and self._lists == other._lists
    and self._properties == other._properties
)

[antony0016]

Can here do early return here to reduce indent and follow how catalog_change.py did?

if not isinstance(other, IdentityPartitionImpl):
    return False
return (
    self._name == other._name
    and self._field_names == other._field_names
    and self._values == other._values
    and self._properties == other._properties
)

[unknowntpo]

okay, early return is better.

[antony0016]

Maybe we should add some test cases for the __eq__ method in the unittest? Here's an example:

partition1 = Partitions.range("p1", Literals.NULL, Literals.integer_literal(6), {})
partition2 = Partitions.range("p1", Literals.NULL, Literals.integer_literal(6), {})
partition3 = Partitions.range("p2", Literals.NULL, Literals.integer_literal(10), {})

# Test same objects are equal
self.assertEqual(partition1, partition2)  # Should be equal
self.assertNotEqual(partition1, partition3)  # Should not be equal

# Test different objects are not equal
partition4 = Partitions.range("p1", Literals.NULL, Literals.integer_literal(10), {})
self.assertNotEqual(partition1, partition4)

# Test comparison with different types
self.assertNotEqual(partition1, "not_a_partition")  # Different type
self.assertNotEqual(partition1, None)  # NoneType

[unknowntpo]

@antony0016 fixed, please take a look.

[antony0016]

LGTM! Thanks for accepting my suggestion!