types use new for generics. no metaprogramming #4016

CircArgs · 2022-01-31T20:39:13Z

This PR is in contrast to #3981 which sought to use a pattern similar to the python typings e.g. List, Dict, Union, etc to create types that could be used inside iceberg and also server as types that could be statically checked when used to type code.

After discussions with @samredai and @rdblue I've revised it further so there is no real metaprogramming yet we still get much of the value.

The same syntax as the current code is used to create types:

IntegerType(), StructType( [ NestedField(True, 1, "required_field", StringType()), NestedField(False, 2, "optional_field", IntegerType()), ] )

yet we get == for free (no dedicated __eq__ methods) and can use isinstance to check types instead of issubclass as was the case in #3981.

Take this example:

>>> FixedType(length=8) is FixedType(length=8) # same object in memory
True

>>> str(IntegerType())
integer

>>> IntegerType() is IntegerType() # same object in memory
True

>>> repr(BooleanType())
BooleanType()

>>> repr(StructType(
        [
            NestedField(True, 1, "required_field", StringType()),
            NestedField(False, 2, "optional_field", IntegerType()),
        ]
    ))
StructType(fields=(NestedField(is_optional=True, field_id=1, name='required_field', field_type=StringType(), doc=None), NestedField(is_optional=False, field_id=2, name='optional_field', field_type=IntegerType(), doc=None)))

>>> str(StructType(
        [
            NestedField(True, 1, "required_field", StringType()),
            NestedField(False, 2, "optional_field", IntegerType()),
        ]
    ))
struct<[nestedfield<True, 1, required_field, string, None>, nestedfield<False, 2, optional_field, integer, None>]>

>>> StructType(
        [
            NestedField(True, 1, "required_field", StringType()),
            NestedField(False, 2, "optional_field", IntegerType()),
        ]
    )==StructType(
        [
            NestedField(True, 1, "required_field", StringType()),
            NestedField(False, 2, "optional_field", IntegerType()),
        ]
    )
True 

>>> StructType(
        [
            NestedField(True, 1, "required_field", StringType()),
            NestedField(False, 2, "optional_field", IntegerType()),
        ]
    )==StructType(
        [
            NestedField(True, 0, "required_field", StringType()), # id changed from 1 to 0
            NestedField(False, 2, "optional_field", IntegerType()),
        ]
    ) 
False 

>>> isinstance(StringType(), StringType)
True

This types.py is about 100 lines less code than the current one with greater functionality as described

The centerpiece of this PR is simply the __new__ method on the base IcebergType which checks the attribute _implemented: Dict[Tuple[str, Tuple[Any]], "IcebergType"] which you can see keeps track of IcebergType instances by storing keys to the type's name and attributes (as defined it the init)

Thank you to @samredai and @rdblue for helping to inspire this change.

Note: if this PR is accepted #3981 should be closed

python/src/iceberg/types.py

python/tests/test_types.py

python/src/iceberg/types.py

jun-he · 2022-02-01T18:10:51Z

python/src/iceberg/types.py

+
+    _implemented = {}  # type: ignore
+
+    def __new__(cls):


Wondering if it is better to move this logic into a Singleton class and any class can extend it to be a singleton.

What do you think the benefits of that might be?

Possible reuse later.

I'd probably continue to use Singleton so that you don't have to call object.__new__ and can use super().__new__ instead. That seems safer to me for some reason.

jun-he · 2022-02-01T18:13:03Z

python/src/iceberg/types.py

+        if cls in cls._implemented:
+            return cls._implemented[cls]
+        else:
+            ret = object.__new__(cls)


Do we need to keep a set here? Seems just _instance = None should be OK, right?

it's a dictionary, but I put it there so that get would work otherwise I have to first check for None right?

jun-he · 2022-02-01T18:21:03Z

python/src/iceberg/types.py

+        doc: Optional[str] = None,
+    ):
+        key = is_optional, field_id, name, field_type, doc
+        cls._implemented[key] = cls._implemented.get(key, object.__new__(cls))


In this line, will it always create a new object, object.__new__(cls) and pass it to get method call?

Yeah you're right. Unnecessarily inefficient. changed it now to use or instead

rdblue · 2022-02-02T22:15:05Z

This is looking good, but I just committed @jun-he's change that adds eq since it was simple and unblocks him. Can you rebase and merge in those changes? That has a Singleton class that can be used in a couple places.

add missing docstrings; comments in __new__ icebergtype back to type; new on all classes; manual str, repr remove instantiating a new object each check in __new__ implemented to instances Type back to IcebergType add whitespace before/after Example in docstrings

samredai · 2022-02-03T16:04:19Z

I took a look at this test failure and it looks like it might be transient. I suspect rerunning the 3.8 test should succeed. That being said I noticed that --diff is not included so we don't see any details in the logs on which lines are causing the lint failure so I opened PR #4034 to add that.

CircArgs · 2022-02-03T17:29:13Z

@rdblue Merged in those changes. I think it's in line with the discussion @jun-he and I were having in his PR. Singleton is as it was in his

python/src/iceberg/types.py

rdblue · 2022-02-06T20:57:10Z

python/src/iceberg/types.py

+
+    def __new__(cls, precision: int, scale: int):
+        key = precision, scale
+        cls._instances[key] = cls._instances.get(key) or object.__new__(cls)


Doesn't this need to pass precision and scale in for __init__?

It's not in __new__'s semantics to deal with initialization. It could be done but I think that's bad practice and it's left to __init__

Okay, I understand. So Python is still going to call __init__ after this method on the object that this returns?

Is it a concern that __init__ is called every time this is returned?

rdblue · 2022-02-06T20:59:10Z

python/src/iceberg/types.py


+class NestedField(IcebergType):
+    """equivalent of `NestedField` type from Java implementation"""


Doc strings should use normal sentence case, so this should start with a capital letter.

Also, I don't think that we want to refer to the Java implementation. This represents a field of a struct, a map key, a map value, or a list element. This is where field IDs, names, docs, and nullability are tracked.

python/src/iceberg/types.py

rdblue · 2022-02-06T21:16:32Z

python/src/iceberg/types.py


-class StructType(Type):
-    def __init__(self, fields: list):
+    def __new__(cls, fields: List[NestedField]):


Is it possible to change this to *fields instead of List[NestedField]? That would allow a bit more natural syntax:

s: StructType = StructType(NestedField(True, 1, "col", StringType()))

sure is. looks much better this way. all set now

rdblue · 2022-02-15T16:40:32Z

python/src/iceberg/types.py


+class NestedField(IcebergType):
+    """This represents a field of a struct, a map key, a map value, or a list element. This is where field IDs, names, docs, and nullability are tracked."""


This doc string should be wrapped at 130 characters, right?

rdblue · 2022-02-15T16:42:34Z

python/src/iceberg/types.py

+
+    Example:
+        >>> MapType(key_id=1, key_type=StringType(), value_id=2, value_type=IntegerType(), value_is_optional=True)
+        MapType(key=NestedField(is_optional=False, field_id=1, name='key', field_type=StringType(), doc=None), value=NestedField(is_optional=True, field_id=2, name='value', field_type=IntegerType(), doc=None))


Looks like this also needs to be wrapped to the line length.

rdblue · 2022-02-15T16:43:43Z

python/src/iceberg/types.py

+    ):
+        super().__init__(
+            f"list<{element_type}>",
+            f"ListType(element_is_optional={element_is_optional}, element_id={element_id}, "


Minor: It would be nice if the argument order were consistent. This is slightly misleading because element_is_optional is last if you're passing by position.

python/src/iceberg/types.py

rdblue · 2022-02-15T16:47:56Z

python/src/iceberg/types.py

+
+    _instances: Dict[Tuple[bool, int, str, IcebergType, Optional[str]], "NestedField"] = {}
+
+    def __new__(


Testing this out in ipython, I missed a couple args and got this error message:

__new__() missing 2 required positional arguments: 'name' and 'field_type'

Is there a way to improve that? Or is that helpful enough?

python/src/iceberg/types.py

rdblue · 2022-02-15T16:51:09Z

Thanks, @CircArgs! Just a couple minor fixes to go and then we can get this in. The only semi-major thing is my question about __init__: is that called every time we return a type? Or does Python keep track and only call it once for the object?

CircArgs · 2022-02-17T22:12:49Z

Hey @rdblue the __init__ will be called every time we request an instance of the type. First __new__ will give us the instance (which the custom __new__ on the class or inherited from Singleton will give) and then __init__ will be called. It's true for both the parameterized typed and the non-parameterized ones.

CircArgs · 2022-02-22T02:53:38Z

@CircArgs, FYI. I tried to open a PR against your branch, but I couldn't so I had to create a new PR.

Feel free to pick the changes into your branch if you want to commit the other PR.

Thanks a lot @rdblue. I've pulled in your commits here. Sorry I wasn't quite on the same page with what you were thinking, makes sense with the flag in your changes

samredai · 2022-02-22T03:09:36Z

I commented in the other PR and I see those commits were added here, this LGTM. Thanks @CircArgs and @rdblue!

rdblue · 2022-02-22T17:43:32Z

Thanks, @CircArgs! Great to have this in. @jun-he, can you update the transforms PR? I think that one is next.

github-actions bot added the python label Jan 31, 2022

CircArgs mentioned this pull request Jan 31, 2022

implement __eq__ for iceberg type classes. #3965

Merged

kbendick reviewed Jan 31, 2022

View reviewed changes

python/src/iceberg/types.py Outdated Show resolved Hide resolved

rdblue reviewed Feb 1, 2022

View reviewed changes

python/src/iceberg/types.py Outdated Show resolved Hide resolved

rdblue reviewed Feb 1, 2022

View reviewed changes

python/src/iceberg/types.py Show resolved Hide resolved

rdblue reviewed Feb 1, 2022

View reviewed changes

python/src/iceberg/types.py Outdated Show resolved Hide resolved

rdblue reviewed Feb 1, 2022

View reviewed changes

python/src/iceberg/types.py Show resolved Hide resolved

rdblue reviewed Feb 1, 2022

View reviewed changes

python/src/iceberg/types.py Outdated Show resolved Hide resolved

rdblue reviewed Feb 1, 2022

View reviewed changes

python/tests/test_types.py Outdated Show resolved Hide resolved

CircArgs requested review from jun-he, kbendick and rdblue February 1, 2022 14:03

rdblue reviewed Feb 1, 2022

View reviewed changes

python/src/iceberg/types.py Outdated Show resolved Hide resolved

jun-he reviewed Feb 1, 2022

View reviewed changes

CircArgs and others added 3 commits February 3, 2022 09:00

merge latest upstream

43db126

remove unspotted conflict from struct docstring

9a61225

reformat with tox

20cafe0

CircArgs requested a review from rdblue February 4, 2022 17:07

rdblue reviewed Feb 6, 2022

View reviewed changes

python/src/iceberg/types.py Outdated Show resolved Hide resolved

rdblue reviewed Feb 6, 2022

View reviewed changes

python/src/iceberg/types.py Outdated Show resolved Hide resolved

rdblue reviewed Feb 6, 2022

View reviewed changes

python/src/iceberg/types.py Show resolved Hide resolved

rdblue reviewed Feb 6, 2022

View reviewed changes

python/src/iceberg/types.py Show resolved Hide resolved

rdblue reviewed Feb 6, 2022

View reviewed changes

python/src/iceberg/types.py Outdated Show resolved Hide resolved

rdblue reviewed Feb 6, 2022

View reviewed changes

struct takes field args instead of list

f9ba3d9

CircArgs requested a review from rdblue February 7, 2022 16:21

CircArgs and others added 7 commits February 7, 2022 11:28

fix docstring formatting and new keys explicit tuples

174c223

correct primitive type inheritance

f7f24f6

tests for is_primitive

32ed6d5

finish primitives tests

c0494ac

List, Map take fields directly rather than NestedField

852902c

remove new from primitivetype

73ee9d7

Merge branch 'master' into types-with-new

f5c20e2

CircArgs requested a review from jun-he February 14, 2022 15:10

run all tox

760c88f

rdblue reviewed Feb 15, 2022

View reviewed changes

python/src/iceberg/types.py Outdated Show resolved Hide resolved

rdblue reviewed Feb 15, 2022

View reviewed changes

python/src/iceberg/types.py Outdated Show resolved Hide resolved

rdblue reviewed Feb 15, 2022

View reviewed changes

python/src/iceberg/types.py Outdated Show resolved Hide resolved

Fix remaining issues.

85e04f9

rdblue mentioned this pull request Feb 20, 2022

Python: Reuse types with __new__ #4174

Closed

rdblue added 2 commits February 20, 2022 11:12

Revert accidental change.

f5f456a

Fix formatting for python 3.8.

5b7c4bc

rdblue merged commit 818d9a5 into apache:master Feb 22, 2022

arminnajafi pushed a commit to arminnajafi/iceberg that referenced this pull request Feb 23, 2022

Python: Reuse types with __new__ (apache#4016)

2512c24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

types use new for generics. no metaprogramming #4016

types use new for generics. no metaprogramming #4016

CircArgs commented Jan 31, 2022 •

edited

Loading

jun-he Feb 1, 2022

CircArgs Feb 1, 2022

rdblue Feb 6, 2022

rdblue Feb 6, 2022

jun-he Feb 1, 2022

CircArgs Feb 1, 2022

jun-he Feb 1, 2022

CircArgs Feb 1, 2022

rdblue commented Feb 2, 2022

samredai commented Feb 3, 2022

CircArgs commented Feb 3, 2022

rdblue Feb 6, 2022

CircArgs Feb 9, 2022

rdblue Feb 15, 2022 •

edited

Loading

rdblue Feb 6, 2022 •

edited

Loading

rdblue Feb 6, 2022

CircArgs Feb 7, 2022

rdblue Feb 15, 2022

rdblue Feb 15, 2022

rdblue Feb 15, 2022

rdblue Feb 15, 2022

rdblue commented Feb 15, 2022

CircArgs commented Feb 17, 2022

CircArgs commented Feb 22, 2022

samredai commented Feb 22, 2022

rdblue commented Feb 22, 2022


		class NestedField(IcebergType):
		"""equivalent of `NestedField` type from Java implementation"""


		class NestedField(IcebergType):
		"""This represents a field of a struct, a map key, a map value, or a list element. This is where field IDs, names, docs, and nullability are tracked."""


		_instances: Dict[Tuple[bool, int, str, IcebergType, Optional[str]], "NestedField"] = {}

		def __new__(

types use new for generics. no metaprogramming #4016

types use new for generics. no metaprogramming #4016

Conversation

CircArgs commented Jan 31, 2022 • edited Loading

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

rdblue commented Feb 2, 2022

samredai commented Feb 3, 2022

CircArgs commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

rdblue Feb 6, 2022 • edited Loading

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

rdblue commented Feb 15, 2022

CircArgs commented Feb 17, 2022

CircArgs commented Feb 22, 2022

samredai commented Feb 22, 2022

rdblue commented Feb 22, 2022

CircArgs commented Jan 31, 2022 •

edited

Loading

rdblue Feb 15, 2022 •

edited

Loading

rdblue Feb 6, 2022 •

edited

Loading