You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "HonahX (via GitHub)" <gi...@apache.org> on 2023/08/09 08:43:20 UTC
[GitHub] [iceberg] HonahX opened a new pull request, #8267: Python: Change UUID representation to bytes and add integration tests
HonahX opened a new pull request, #8267:
URL: https://github.com/apache/iceberg/pull/8267
Fixes: #8248
This PR changes the value stored in UUID literal from `uuid` to `bytes` and add relevant integration tests for unpartitined uuid tables
--
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] HonahX commented on a diff in pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #8267:
URL: https://github.com/apache/iceberg/pull/8267#discussion_r1288168026
##########
python/pyiceberg/io/pyarrow.py:
##########
@@ -451,7 +451,7 @@ def visit_binary(self, _: BinaryType) -> pa.DataType:
def _convert_scalar(value: Any, iceberg_type: IcebergType) -> pa.scalar:
if not isinstance(iceberg_type, PrimitiveType):
raise ValueError(f"Expected primitive type, got: {iceberg_type}")
- return pa.scalar(value).cast(schema_to_pyarrow(iceberg_type))
+ return pa.scalar(value=value, type=schema_to_pyarrow(iceberg_type))
Review Comment:
Changing this because I hit:
```
pyarrow.lib.ArrowNotImplementedError: casting scalars of type binary to type fixed_size_binary[25]
```
when trying to apply a row filter on either UUIDType or FixedType:
```python
fixed_table = catalog.load_table("default.test_uuid_and_fixed_unpartitioned")
arrow_table = fixed_table.scan(row_filter=EqualTo("fixed_col", b"1234567890123456789012345")).to_arrow()
```
Seems `pa.scalar` can specify type during creation and doing so can solve this problem. However, I am not sure if there is any implication for this change. Please let me know if there is any issue with this change. Thank you in advance.
--
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] HonahX commented on a diff in pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #8267:
URL: https://github.com/apache/iceberg/pull/8267#discussion_r1289678800
##########
python/pyiceberg/expressions/literals.py:
##########
@@ -139,7 +139,7 @@ def literal(value: L) -> Literal[L]:
elif isinstance(value, str):
return StringLiteral(value)
elif isinstance(value, UUID):
- return UUIDLiteral(value)
+ return UUIDLiteral(value) # type: ignore
Review Comment:
Seems the console conversion from string to uuid fails because `to` expect a type instance:
`l.to(UUIDType())`
--
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] Fokko commented on a diff in pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8267:
URL: https://github.com/apache/iceberg/pull/8267#discussion_r1289601328
##########
python/pyiceberg/expressions/literals.py:
##########
@@ -139,7 +139,7 @@ def literal(value: L) -> Literal[L]:
elif isinstance(value, str):
return StringLiteral(value)
elif isinstance(value, UUID):
- return UUIDLiteral(value)
+ return UUIDLiteral(value) # type: ignore
Review Comment:
So, the `UUIDLiteral` is not intended to be used by the user. The user would pass in:
```python
>>> l = literal('27f6397c-65b9-4464-a2f5-f4ee81629134')
StringLiteral('27f6397c-65b9-4464-a2f5-f4ee81629134')
```
And then internally, when binding to the schema, it will be converted into an UUID, which is failing right now:
```python
>>> from pyiceberg.types import UUIDType
>>> l.to(UUIDType)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/functools.py", line 914, in _method
return method.__get__(obj, cls)(*args, **kwargs)
File "/Users/fokkodriesprong/Desktop/iceberg/python/pyiceberg/expressions/literals.py", line 519, in to
raise TypeError(f"Cannot convert StringLiteral into {type_var}")
TypeError: Cannot convert StringLiteral into <class 'pyiceberg.types.UUIDType'>
```
Which means that we need to implement this one as well.
The same is for the bytes:
```python
>>> from uuid import UUID
>>> u = UUID('27f6397c-65b9-4464-a2f5-f4ee81629134')
>>> l = literal(u.bytes)
>>> l
BinaryLiteral(b"'\xf69|e\xb9Dd\xa2\xf5\xf4\xee\x81b\x914")
>>> l.to(UUIDType)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/functools.py", line 914, in _method
return method.__get__(obj, cls)(*args, **kwargs)
File "/Users/fokkodriesprong/Desktop/iceberg/python/pyiceberg/expressions/literals.py", line 640, in to
raise TypeError(f"Cannot convert BinaryLiteral into {type_var}")
TypeError: Cannot convert BinaryLiteral into <class 'pyiceberg.types.UUIDType'>
```
I think we need to extend the literal constructor with many more types, such as `datetime`, `date`, `time`, `UUID`, etc. For the datetime related, I've created a separate issue: https://github.com/apache/iceberg/issues/8282 Maybe we can add `UUID` to `literal(..)` in this PR to avoid backward incompatibility.
--
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] Fokko commented on pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #8267:
URL: https://github.com/apache/iceberg/pull/8267#issuecomment-1672774076
Thanks @HonahX for fixing this and also smoothing out the code 👍🏻
--
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] Fokko commented on a diff in pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8267:
URL: https://github.com/apache/iceberg/pull/8267#discussion_r1288236661
##########
python/pyiceberg/io/pyarrow.py:
##########
@@ -451,7 +451,7 @@ def visit_binary(self, _: BinaryType) -> pa.DataType:
def _convert_scalar(value: Any, iceberg_type: IcebergType) -> pa.scalar:
if not isinstance(iceberg_type, PrimitiveType):
raise ValueError(f"Expected primitive type, got: {iceberg_type}")
- return pa.scalar(value).cast(schema_to_pyarrow(iceberg_type))
+ return pa.scalar(value=value, type=schema_to_pyarrow(iceberg_type))
Review Comment:
I'm not aware of the exact semantics of this change, but I've added a couple of tests recently that cover this, so I think we're good.
--
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] HonahX commented on a diff in pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #8267:
URL: https://github.com/apache/iceberg/pull/8267#discussion_r1289664294
##########
python/pyiceberg/expressions/literals.py:
##########
@@ -139,7 +139,7 @@ def literal(value: L) -> Literal[L]:
elif isinstance(value, str):
return StringLiteral(value)
elif isinstance(value, UUID):
- return UUIDLiteral(value)
+ return UUIDLiteral(value) # type: ignore
Review Comment:
Thanks for the explanation! I added conversions from `BinaryLiteral` to `UUID` and from `FixedLiteral` to `UUID`, and related tests.
Here is a summarization of the supported conversion now:
```python
from pyiceberg.expressions.literals import *
from pyiceberg.types import *
import uuid
test_uuid = uuid.UUID('d4ccb06d-e7de-48e6-ad05-649fe8deda50')
lu = literal(test_uuid)
ls = literal('d4ccb06d-e7de-48e6-ad05-649fe8deda50')
lbinary = literal(test_uuid.bytes)
lfixed = literal(test_uuid.bytes).to(FixedType(16))
assert lu.value == test_uuid.bytes
assert ls.to(UUIDType()) == lu
assert lbinary.to(UUIDType()) == lu
assert lfixed.to(UUIDType()) == lu
```
--
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] Fokko commented on a diff in pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8267:
URL: https://github.com/apache/iceberg/pull/8267#discussion_r1288298950
##########
python/dev/Dockerfile:
##########
@@ -65,6 +65,8 @@ RUN chmod u+x /opt/spark/sbin/* && \
RUN pip3 install -q ipython
+RUN pip3 install 'pyiceberg[pyarrow,duckdb,pandas]==0.4.0'
Review Comment:
Ah, I see, because we now use the catalog in the provision script.
```suggestion
RUN pip3 install 'pyiceberg==0.4.0'
```
We only need the basics to create a table. This should reduce the image by a couple of hundred megabytes
--
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] Fokko commented on a diff in pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8267:
URL: https://github.com/apache/iceberg/pull/8267#discussion_r1288262545
##########
python/pyiceberg/conversions.py:
##########
@@ -228,7 +228,9 @@ def _(_: StringType, value: str) -> bytes:
@to_bytes.register(UUIDType)
-def _(_: UUIDType, value: uuid.UUID) -> bytes:
+def _(_: UUIDType, value: Union[uuid.UUID, bytes]) -> bytes:
+ if isinstance(value, bytes):
+ return value
return _UUID_STRUCT.pack((value.int >> 64) & 0xFFFFFFFFFFFFFFFF, value.int & 0xFFFFFFFFFFFFFFFF)
Review Comment:
Slightly related, I think we can do:
```suggestion
return value.bytes
```
```
>>> import uuid
>>> from struct import Struct
>>> _UUID_STRUCT = Struct(">QQ")
>>> u = uuid.uuid4()
>>> u
UUID('43c6697a-4d50-44ee-806e-edf58b08430e')
>>> u.bytes
b'C\xc6izMPD\xee\x80n\xed\xf5\x8b\x08C\x0e'
>>> _UUID_STRUCT.pack((u.int >> 64) & 0xFFFFFFFFFFFFFFFF, u.int & 0xFFFFFFFFFFFFFFFF)
b'C\xc6izMPD\xee\x80n\xed\xf5\x8b\x08C\x0e'
```
##########
python/dev/Dockerfile:
##########
@@ -65,6 +65,8 @@ RUN chmod u+x /opt/spark/sbin/* && \
RUN pip3 install -q ipython
+RUN pip3 install 'pyiceberg[pyarrow,duckdb,pandas]==0.4.0'
Review Comment:
This should not be required?
##########
python/pyiceberg/expressions/literals.py:
##########
@@ -139,7 +139,7 @@ def literal(value: L) -> Literal[L]:
elif isinstance(value, str):
return StringLiteral(value)
elif isinstance(value, UUID):
- return UUIDLiteral(value)
+ return UUIDLiteral(value) # type: ignore
Review Comment:
```suggestion
return UUIDLiteral(value.bytes)
```
--
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] HonahX commented on a diff in pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #8267:
URL: https://github.com/apache/iceberg/pull/8267#discussion_r1289565779
##########
python/pyiceberg/expressions/literals.py:
##########
@@ -139,7 +139,7 @@ def literal(value: L) -> Literal[L]:
elif isinstance(value, str):
return StringLiteral(value)
elif isinstance(value, UUID):
- return UUIDLiteral(value)
+ return UUIDLiteral(value) # type: ignore
Review Comment:
Thanks for your suggestion. I tried this and it turned out that the error here:
```
Incompatible return value type (got "UUIDLiteral", expected "Literal[UUID]") [return-value]
```
is due to the `literal` function expecting a return value of `Literal(<Type of value>)`.
Hence, we need this ignore since UUIDLiteral is now a `Literal[bytes]`
I chose to let the initialization of `UUIDLiteral` still take `UUID` as input to maintain minimal changes to initialization code in other places. I think letting `UUIDLiteral.__init__` takes `bytes` or `Union[bytes, uuid]` is also reasonable. Please let me know if you prefer other way of initialization this class.
--
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] HonahX commented on a diff in pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #8267:
URL: https://github.com/apache/iceberg/pull/8267#discussion_r1289556725
##########
python/dev/Dockerfile:
##########
@@ -65,6 +65,8 @@ RUN chmod u+x /opt/spark/sbin/* && \
RUN pip3 install -q ipython
+RUN pip3 install 'pyiceberg[pyarrow,duckdb,pandas]==0.4.0'
Review Comment:
Thanks for the suggestion. It turns out that we need one additional package `s3fs` for minio. But it only takes ~200KB
```bash
site-packages git:(stable) du -sh s3fs
200K s3fs
```
--
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] Fokko commented on a diff in pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8267:
URL: https://github.com/apache/iceberg/pull/8267#discussion_r1288298950
##########
python/dev/Dockerfile:
##########
@@ -65,6 +65,8 @@ RUN chmod u+x /opt/spark/sbin/* && \
RUN pip3 install -q ipython
+RUN pip3 install 'pyiceberg[pyarrow,duckdb,pandas]==0.4.0'
Review Comment:
Ah, I see, because we now use the catalog in the provision script.
```suggestion
RUN pip3 install 'pyiceberg==0.4.0'
```
--
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] Fokko commented on a diff in pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8267:
URL: https://github.com/apache/iceberg/pull/8267#discussion_r1289601328
##########
python/pyiceberg/expressions/literals.py:
##########
@@ -139,7 +139,7 @@ def literal(value: L) -> Literal[L]:
elif isinstance(value, str):
return StringLiteral(value)
elif isinstance(value, UUID):
- return UUIDLiteral(value)
+ return UUIDLiteral(value) # type: ignore
Review Comment:
So, the `UUIDLiteral` is not intended to be used by the user. The user would pass in:
```python
>>> l = literal('27f6397c-65b9-4464-a2f5-f4ee81629134')
StringLiteral('27f6397c-65b9-4464-a2f5-f4ee81629134')
```
And then internally, when binding to the schema, it will be converted into an UUID, which is failing right now:
```python
>>> from pyiceberg.types import UUIDType
>>> l.to(UUIDType)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/functools.py", line 914, in _method
return method.__get__(obj, cls)(*args, **kwargs)
File "/Users/fokkodriesprong/Desktop/iceberg/python/pyiceberg/expressions/literals.py", line 519, in to
raise TypeError(f"Cannot convert StringLiteral into {type_var}")
TypeError: Cannot convert StringLiteral into <class 'pyiceberg.types.UUIDType'>
```
Which means that we need to implement this one as well.
The same is for the bytes:
```python
>>> from uuid import UUID
>>> u = UUID('27f6397c-65b9-4464-a2f5-f4ee81629134')
>>> l = literal(u.bytes)
>>> l
BinaryLiteral(b"'\xf69|e\xb9Dd\xa2\xf5\xf4\xee\x81b\x914")
>>> l.to(UUIDType)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/functools.py", line 914, in _method
return method.__get__(obj, cls)(*args, **kwargs)
File "/Users/fokkodriesprong/Desktop/iceberg/python/pyiceberg/expressions/literals.py", line 640, in to
raise TypeError(f"Cannot convert BinaryLiteral into {type_var}")
TypeError: Cannot convert BinaryLiteral into <class 'pyiceberg.types.UUIDType'>
```
I think we need to extend the literal constructor with many more types, such as `datetime`, `date`, `time`, `UUID`, etc.
--
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] HonahX commented on a diff in pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #8267:
URL: https://github.com/apache/iceberg/pull/8267#discussion_r1289664294
##########
python/pyiceberg/expressions/literals.py:
##########
@@ -139,7 +139,7 @@ def literal(value: L) -> Literal[L]:
elif isinstance(value, str):
return StringLiteral(value)
elif isinstance(value, UUID):
- return UUIDLiteral(value)
+ return UUIDLiteral(value) # type: ignore
Review Comment:
Thanks for the explanation! I added conversions from `BinaryLiteral` to `UUID` and from `FixedLiteral` to `UUID`, and related tests.
Here is a summarization of the supported conversion now:
```python
from pyiceberg.expressions.literals import *
from pyiceberg.types import *
import uuid
test_uuid = uuid.UUID('d4ccb06d-e7de-48e6-ad05-649fe8deda50')
lu = literal(test_uuid)
ls = literal('d4ccb06d-e7de-48e6-ad05-649fe8deda50')
lbinary = literal(test_uuid.bytes)
lfixed = literal(test_uuid.bytes).to(FixedType(16))
assert lu.value == test_uuid.bytes
assert ls.to(UUIDType()) == lu
assert lbinary.to(UUIDType()) == lu
assert lfixed.to(UUIDType()) == lu
```
For conversion from `StringLiteral` to `UUID`, I think It fails because the console mode does not work well with `@singledispatchmethod`
For example:
```python
>>> i = literal('1')
>>> i.to(IntegerType)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/functools.py", line 914, in _method
return method.__get__(obj, cls)(*args, **kwargs)
File "/Users/jonasjiang/Workspace/Apache_Iceberg_ws/iceberg_python/python/pyiceberg/expressions/literals.py", line 519, in to
raise TypeError(f"Cannot convert StringLiteral into {type_var}")
TypeError: Cannot convert StringLiteral into <class 'pyiceberg.types.IntegerType'>
```
the conversion from string to int should work but it also fails
--
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] Fokko merged pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #8267:
URL: https://github.com/apache/iceberg/pull/8267
--
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] HonahX commented on a diff in pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #8267:
URL: https://github.com/apache/iceberg/pull/8267#discussion_r1289558436
##########
python/pyiceberg/transforms.py:
##########
@@ -269,13 +270,9 @@ def hash_func(v: Any) -> int:
elif source_type == UUIDType:
def hash_func(v: Any) -> int:
- return mmh3.hash(
- struct.pack(
- ">QQ",
- (v.int >> 64) & 0xFFFFFFFFFFFFFFFF,
- v.int & 0xFFFFFFFFFFFFFFFF,
- )
- )
+ if isinstance(v, UUID):
+ return mmh3.hash(v.bytes)
+ return mmh3.hash(v)
Review Comment:
I found another place that could be simplified based on the above comment. Also, I think we need to let the hash function handles `bytes` since `transform` usually calls on literal's value and now UUIDLiteral stores `bytes`
--
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] Fokko commented on a diff in pull request #8267: Python: Change UUID representation to bytes and add integration tests
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8267:
URL: https://github.com/apache/iceberg/pull/8267#discussion_r1289609204
##########
python/pyiceberg/transforms.py:
##########
@@ -269,13 +270,9 @@ def hash_func(v: Any) -> int:
elif source_type == UUIDType:
def hash_func(v: Any) -> int:
- return mmh3.hash(
- struct.pack(
- ">QQ",
- (v.int >> 64) & 0xFFFFFFFFFFFFFFFF,
- v.int & 0xFFFFFFFFFFFFFFFF,
- )
- )
+ if isinstance(v, UUID):
+ return mmh3.hash(v.bytes)
+ return mmh3.hash(v)
Review Comment:
Awesome, thanks for fixing this, much better 👍🏻
##########
python/pyiceberg/transforms.py:
##########
@@ -269,13 +270,9 @@ def hash_func(v: Any) -> int:
elif source_type == UUIDType:
def hash_func(v: Any) -> int:
- return mmh3.hash(
- struct.pack(
- ">QQ",
- (v.int >> 64) & 0xFFFFFFFFFFFFFFFF,
- v.int & 0xFFFFFFFFFFFFFFFF,
- )
- )
+ if isinstance(v, UUID):
+ return mmh3.hash(v.bytes)
+ return mmh3.hash(v)
Review Comment:
Awesome, thanks for fixing this, much better 👍🏻
--
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