You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/04/23 02:57:00 UTC
[GitHub] [iceberg] samredai opened a new pull request, #4619: Add BoundReference class
samredai opened a new pull request, #4619:
URL: https://github.com/apache/iceberg/pull/4619
This adds a `BoundReference` class that takes a field and an accessor and allows you to evaluate a struct (a row object that abides by the `StructProtocol`) to retrieve the value at the referenced field's position.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#discussion_r857160261
##########
python/src/iceberg/expressions/base.py:
##########
@@ -147,3 +148,61 @@ def get(self, container: StructProtocol) -> Any:
Any: The value at position `self.position` in the container
"""
return container.get(self.position)
+
+
+class BoundReference:
+ """A reference bound to a field with an accessor for acquiring the field's value
Review Comment:
Accessors are internal, so there is no need to mention them in docs. This doesn't necessarily need to use an accessor to work.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#discussion_r857160370
##########
python/src/iceberg/expressions/base.py:
##########
@@ -147,3 +148,61 @@ def get(self, container: StructProtocol) -> Any:
Any: The value at position `self.position` in the container
"""
return container.get(self.position)
+
+
+class BoundReference:
+ """A reference bound to a field with an accessor for acquiring the field's value
+
+ Args:
+ field (NestedField): A referenced field in an Iceberg schema
+ accessor (Accessor): An Accessor object to access the value at the field's position
+ """
+
+ def __init__(self, field: NestedField, accessor: Accessor):
+ self._field = field
+ self._accessor = accessor
+
+ def __str__(self):
+ return f"ref(id={self.field_id})"
+
+ def __repr__(self):
+ return f"BoundReference(field={repr(self.field)}, accessor={repr(self.accessor)})"
Review Comment:
Accessor doesn't have a repr. We should either add it or not use it here.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a diff in pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#discussion_r857183664
##########
python/src/iceberg/expressions/base.py:
##########
@@ -147,3 +148,61 @@ def get(self, container: StructProtocol) -> Any:
Any: The value at position `self.position` in the container
"""
return container.get(self.position)
+
+
+class BoundReference:
+ """A reference bound to a field with an accessor for acquiring the field's value
+
+ Args:
+ field (NestedField): A referenced field in an Iceberg schema
+ accessor (Accessor): An Accessor object to access the value at the field's position
+ """
+
+ def __init__(self, field: NestedField, accessor: Accessor):
+ self._field = field
+ self._accessor = accessor
+
+ def __str__(self):
+ return f"ref(id={self.field_id})"
+
+ def __repr__(self):
+ return f"BoundReference(field={repr(self.field)}, accessor={repr(self.accessor)})"
+
+ @property
+ def field(self) -> NestedField:
+ """The referenced field"""
+ return self._field
+
+ @property
+ def field_id(self) -> int:
+ """The ID referenced field"""
+ return self._field.field_id
+
+ @property
+ def field_type(self) -> IcebergType:
+ """The type of the referenced field"""
+ return self._field.type
+
+ @property
+ def accessor(self) -> Accessor:
+ """The accessor for retrieving the value at the referenced field's position"""
+ return self._accessor
+
+ def __eq__(self, other) -> bool:
+ if self is other:
+ return True
+ elif not isinstance(other, BoundReference):
+ return False
+
+ return self.field_id == other.field_id and self.field_type == other.field_type
Review Comment:
I removed `__eq__` for now. We can add it later if it's required.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #4619:
URL: https://github.com/apache/iceberg/pull/4619
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#discussion_r857184632
##########
python/tests/expressions/test_expressions_base.py:
##########
@@ -217,3 +217,43 @@ def test_accessor_base_class(foo_struct):
assert base.Accessor(position=9).get(foo_struct) == True
assert base.Accessor(position=10).get(foo_struct) == False
assert base.Accessor(position=11).get(foo_struct) == b"\x19\x04\x9e?"
+
+
+def test_bound_reference_str_and_repr():
+ """Test str and repr of BoundReference"""
+ field = NestedField(field_id=1, name="foo", field_type=StringType(), is_optional=False)
+ position1_accessor = base.Accessor(position=1)
+ bound_ref = base.BoundReference(field=field, accessor=position1_accessor)
+ assert str(bound_ref) == f"BoundReference(field={repr(field)}, accessor={repr(position1_accessor)})"
+ assert repr(bound_ref) == f"BoundReference(field={repr(field)}, accessor={repr(position1_accessor)})"
+
+
+def test_bound_reference_field_property():
+ """Test str and repr of BoundReference"""
+ field = NestedField(field_id=1, name="foo", field_type=StringType(), is_optional=False)
+ position1_accessor = base.Accessor(position=1)
+ bound_ref = base.BoundReference(field=field, accessor=position1_accessor)
+ assert bound_ref.field == NestedField(field_id=1, name="foo", field_type=StringType(), is_optional=False)
Review Comment:
I'm not sure I'd expose `field`, but it should be fine for now.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#discussion_r857161003
##########
python/src/iceberg/expressions/base.py:
##########
@@ -147,3 +148,61 @@ def get(self, container: StructProtocol) -> Any:
Any: The value at position `self.position` in the container
"""
return container.get(self.position)
+
+
+class BoundReference:
+ """A reference bound to a field with an accessor for acquiring the field's value
+
+ Args:
+ field (NestedField): A referenced field in an Iceberg schema
+ accessor (Accessor): An Accessor object to access the value at the field's position
+ """
+
+ def __init__(self, field: NestedField, accessor: Accessor):
+ self._field = field
+ self._accessor = accessor
+
+ def __str__(self):
+ return f"ref(id={self.field_id})"
+
+ def __repr__(self):
+ return f"BoundReference(field={repr(self.field)}, accessor={repr(self.accessor)})"
+
+ @property
+ def field(self) -> NestedField:
+ """The referenced field"""
+ return self._field
+
+ @property
+ def field_id(self) -> int:
+ """The ID referenced field"""
+ return self._field.field_id
+
+ @property
+ def field_type(self) -> IcebergType:
+ """The type of the referenced field"""
+ return self._field.type
+
+ @property
+ def accessor(self) -> Accessor:
+ """The accessor for retrieving the value at the referenced field's position"""
+ return self._accessor
+
+ def __eq__(self, other) -> bool:
+ if self is other:
+ return True
+ elif not isinstance(other, BoundReference):
+ return False
+
+ return self.field_id == other.field_id and self.field_type == other.field_type
Review Comment:
I don't think I agree with this definition of equality. I think that equality comes down to the accessor (position) and the type if we were to implement it... But we may want to be more strict and include the entire field, not just field ID.
We purposely don't implement equality in the JVM implementation because it's weird to define what "equals" means for an expression: is it that the expression produces the same result? Or is it the exact same expression including the referenced field? If it is the latter, isn't that restrictive because we don't really need the field in this class, just the type and the accessor?
Rather than answer those questions, we test using the behavior and the exposed API fields -- basically just `field_type` and `eval`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
samredai commented on PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#issuecomment-1107336380
cc: @dramaticlly @CircArgs @jun-he @dhruv-pratap
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#discussion_r857160465
##########
python/src/iceberg/expressions/base.py:
##########
@@ -147,3 +148,61 @@ def get(self, container: StructProtocol) -> Any:
Any: The value at position `self.position` in the container
"""
return container.get(self.position)
+
+
+class BoundReference:
+ """A reference bound to a field with an accessor for acquiring the field's value
+
+ Args:
+ field (NestedField): A referenced field in an Iceberg schema
+ accessor (Accessor): An Accessor object to access the value at the field's position
+ """
+
+ def __init__(self, field: NestedField, accessor: Accessor):
+ self._field = field
+ self._accessor = accessor
+
+ def __str__(self):
+ return f"ref(id={self.field_id})"
+
+ def __repr__(self):
+ return f"BoundReference(field={repr(self.field)}, accessor={repr(self.accessor)})"
+
+ @property
+ def field(self) -> NestedField:
+ """The referenced field"""
+ return self._field
+
+ @property
+ def field_id(self) -> int:
+ """The ID referenced field"""
+ return self._field.field_id
+
+ @property
+ def field_type(self) -> IcebergType:
+ """The type of the referenced field"""
+ return self._field.type
+
+ @property
+ def accessor(self) -> Accessor:
Review Comment:
Why expose all of these properties? It seems simpler just to expose `eval`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a diff in pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#discussion_r857183249
##########
python/src/iceberg/expressions/base.py:
##########
@@ -147,3 +148,61 @@ def get(self, container: StructProtocol) -> Any:
Any: The value at position `self.position` in the container
"""
return container.get(self.position)
+
+
+class BoundReference:
+ """A reference bound to a field with an accessor for acquiring the field's value
+
+ Args:
+ field (NestedField): A referenced field in an Iceberg schema
+ accessor (Accessor): An Accessor object to access the value at the field's position
+ """
+
+ def __init__(self, field: NestedField, accessor: Accessor):
+ self._field = field
+ self._accessor = accessor
+
+ def __str__(self):
+ return f"ref(id={self.field_id})"
+
+ def __repr__(self):
+ return f"BoundReference(field={repr(self.field)}, accessor={repr(self.accessor)})"
Review Comment:
Thanks for catching this, I added a repr to Accessor.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] CircArgs commented on a diff in pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
CircArgs commented on code in PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#discussion_r857154589
##########
python/src/iceberg/expressions/base.py:
##########
@@ -147,3 +148,61 @@ def get(self, container: StructProtocol) -> Any:
Any: The value at position `self.position` in the container
"""
return container.get(self.position)
+
+
+class BoundReference:
+ """A reference bound to a field with an accessor for acquiring the field's value
+
+ Args:
+ field (NestedField): A referenced field in an Iceberg schema
+ accessor (Accessor): An Accessor object to access the value at the field's position
+ """
+
+ def __init__(self, field: NestedField, accessor: Accessor):
+ self._field = field
+ self._accessor = accessor
+
+ def __str__(self):
+ return f"ref(id={self.field_id})"
+
+ def __repr__(self):
+ return f"BoundReference(field={repr(self.field)}, accessor={repr(self.accessor)})"
+
+ @property
+ def field(self) -> NestedField:
+ """The referenced field"""
+ return self._field
+
+ @property
+ def field_id(self) -> int:
+ """The ID referenced field"""
+ return self._field.field_id
+
+ @property
+ def field_type(self) -> IcebergType:
+ """The type of the referenced field"""
+ return self._field.type
+
+ @property
+ def accessor(self) -> Accessor:
+ """The accessor for retrieving the value at the referenced field's position"""
+ return self._accessor
+
+ def __eq__(self, other) -> bool:
+ if id(self) == id(other):
+ return True
+ elif other is None or not isinstance(other, BoundReference):
+ return False
Review Comment:
If you try to compare to another BoundReference here with a different `id` then doesn't this return `None`?: Misses the first branch, it's not None but is BoundReference -> No more branches, function returns None.
not sure if this is desired to just boil down to `self is other` (I think equivalent to the first branch) e.g. the default `__eq__` if I'm not mistaken, in which case we don't even need to implement this
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#issuecomment-1107927098
Thanks, @samredai!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a diff in pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#discussion_r857183626
##########
python/src/iceberg/expressions/base.py:
##########
@@ -147,3 +148,61 @@ def get(self, container: StructProtocol) -> Any:
Any: The value at position `self.position` in the container
"""
return container.get(self.position)
+
+
+class BoundReference:
+ """A reference bound to a field with an accessor for acquiring the field's value
+
+ Args:
+ field (NestedField): A referenced field in an Iceberg schema
+ accessor (Accessor): An Accessor object to access the value at the field's position
+ """
+
+ def __init__(self, field: NestedField, accessor: Accessor):
+ self._field = field
+ self._accessor = accessor
+
+ def __str__(self):
+ return f"ref(id={self.field_id})"
+
+ def __repr__(self):
+ return f"BoundReference(field={repr(self.field)}, accessor={repr(self.accessor)})"
+
+ @property
+ def field(self) -> NestedField:
+ """The referenced field"""
+ return self._field
+
+ @property
+ def field_id(self) -> int:
+ """The ID referenced field"""
+ return self._field.field_id
+
+ @property
+ def field_type(self) -> IcebergType:
+ """The type of the referenced field"""
+ return self._field.type
+
+ @property
+ def accessor(self) -> Accessor:
Review Comment:
Updated this to just expose `eval` and `field`. Should I remove the `field` property as well?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a diff in pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#discussion_r857183547
##########
python/src/iceberg/expressions/base.py:
##########
@@ -147,3 +148,61 @@ def get(self, container: StructProtocol) -> Any:
Any: The value at position `self.position` in the container
"""
return container.get(self.position)
+
+
+class BoundReference:
+ """A reference bound to a field with an accessor for acquiring the field's value
Review Comment:
I updated this description to "A reference bound to a field in a schema" but I'm not sure I fully understand what you mean by:
> This doesn't necessarily need to use an accessor to work.
Can you describe how this would be used without an accessor? I know this class won't be exposed at the top level API but I'd imagine a developer working in this area of the code would find a description of the relationship to accessors as helpful.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#discussion_r857184734
##########
python/src/iceberg/expressions/base.py:
##########
@@ -147,3 +148,61 @@ def get(self, container: StructProtocol) -> Any:
Any: The value at position `self.position` in the container
"""
return container.get(self.position)
+
+
+class BoundReference:
+ """A reference bound to a field with an accessor for acquiring the field's value
Review Comment:
I don't think it would work without an accessor right now. But you don't need to tie the implementation to an accessor. That's an internal detail.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a diff in pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#discussion_r857156833
##########
python/src/iceberg/expressions/base.py:
##########
@@ -147,3 +148,61 @@ def get(self, container: StructProtocol) -> Any:
Any: The value at position `self.position` in the container
"""
return container.get(self.position)
+
+
+class BoundReference:
+ """A reference bound to a field with an accessor for acquiring the field's value
+
+ Args:
+ field (NestedField): A referenced field in an Iceberg schema
+ accessor (Accessor): An Accessor object to access the value at the field's position
+ """
+
+ def __init__(self, field: NestedField, accessor: Accessor):
+ self._field = field
+ self._accessor = accessor
+
+ def __str__(self):
+ return f"ref(id={self.field_id})"
+
+ def __repr__(self):
+ return f"BoundReference(field={repr(self.field)}, accessor={repr(self.accessor)})"
+
+ @property
+ def field(self) -> NestedField:
+ """The referenced field"""
+ return self._field
+
+ @property
+ def field_id(self) -> int:
+ """The ID referenced field"""
+ return self._field.field_id
+
+ @property
+ def field_type(self) -> IcebergType:
+ """The type of the referenced field"""
+ return self._field.type
+
+ @property
+ def accessor(self) -> Accessor:
+ """The accessor for retrieving the value at the referenced field's position"""
+ return self._accessor
+
+ def __eq__(self, other) -> bool:
+ if id(self) == id(other):
+ return True
+ elif other is None or not isinstance(other, BoundReference):
+ return False
Review Comment:
In the path you described it would fall to the return statement at the end of the function:
```py
return self.field_id == other.field_id and self.field_type == other.field_type
```
I don't yet have full context around all scenarios where we'd compare against a `BoundReference` but the if/else here is just to get the easy cases (True is the id is the same, False if `other` isn't even a `BoundReference`). In the case where the ids differ but `other` is a `BoundReference`, it checks if the field being bound is the same ID and Type.
One thing I'm noticing though is that `elif other is None or not isinstance(other, BoundReference)` is redundant and can be changed to just `elif not isinstance(other, BoundReference)`. Updated it!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samredai commented on a diff in pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
samredai commented on code in PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#discussion_r857158363
##########
python/src/iceberg/expressions/base.py:
##########
@@ -147,3 +148,61 @@ def get(self, container: StructProtocol) -> Any:
Any: The value at position `self.position` in the container
"""
return container.get(self.position)
+
+
+class BoundReference:
+ """A reference bound to a field with an accessor for acquiring the field's value
+
+ Args:
+ field (NestedField): A referenced field in an Iceberg schema
+ accessor (Accessor): An Accessor object to access the value at the field's position
+ """
+
+ def __init__(self, field: NestedField, accessor: Accessor):
+ self._field = field
+ self._accessor = accessor
+
+ def __str__(self):
+ return f"ref(id={self.field_id})"
+
+ def __repr__(self):
+ return f"BoundReference(field={repr(self.field)}, accessor={repr(self.accessor)})"
+
+ @property
+ def field(self) -> NestedField:
+ """The referenced field"""
+ return self._field
+
+ @property
+ def field_id(self) -> int:
+ """The ID referenced field"""
+ return self._field.field_id
+
+ @property
+ def field_type(self) -> IcebergType:
+ """The type of the referenced field"""
+ return self._field.type
+
+ @property
+ def accessor(self) -> Accessor:
+ """The accessor for retrieving the value at the referenced field's position"""
+ return self._accessor
+
+ def __eq__(self, other) -> bool:
+ if id(self) == id(other):
+ return True
+ elif other is None or not isinstance(other, BoundReference):
+ return False
Review Comment:
I just realized your point about how using `is` is the equivalent of the first branch. Updated!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#discussion_r857184689
##########
python/src/iceberg/expressions/base.py:
##########
@@ -147,3 +148,61 @@ def get(self, container: StructProtocol) -> Any:
Any: The value at position `self.position` in the container
"""
return container.get(self.position)
+
+
+class BoundReference:
+ """A reference bound to a field with an accessor for acquiring the field's value
+
+ Args:
+ field (NestedField): A referenced field in an Iceberg schema
+ accessor (Accessor): An Accessor object to access the value at the field's position
+ """
+
+ def __init__(self, field: NestedField, accessor: Accessor):
+ self._field = field
+ self._accessor = accessor
+
+ def __str__(self):
+ return f"ref(id={self.field_id})"
+
+ def __repr__(self):
+ return f"BoundReference(field={repr(self.field)}, accessor={repr(self.accessor)})"
+
+ @property
+ def field(self) -> NestedField:
+ """The referenced field"""
+ return self._field
+
+ @property
+ def field_id(self) -> int:
+ """The ID referenced field"""
+ return self._field.field_id
+
+ @property
+ def field_type(self) -> IcebergType:
+ """The type of the referenced field"""
+ return self._field.type
+
+ @property
+ def accessor(self) -> Accessor:
Review Comment:
I probably would. I think the relevant information is the value type that is produced, not the field info.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4619: Add BoundReference class
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4619:
URL: https://github.com/apache/iceberg/pull/4619#issuecomment-1107889476
Can you rebase this? Looks like there are conflicts.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org