You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "syun64 (via GitHub)" <gi...@apache.org> on 2024/01/25 16:54:17 UTC
[PR] create_table with PyArrow Schema [iceberg-python]
syun64 opened a new pull request, #305:
URL: https://github.com/apache/iceberg-python/pull/305
This addresses: https://github.com/apache/iceberg-python/issues/278
Added `UP037 [*] Remove quotes from type annotation` to ruff in order to support [Forward References: PEP-484](https://peps.python.org/pep-0484/#forward-references)
As discussed in the issue, we are proposing to update the create_table API to:
```
def create_table(
self,
identifier: Union[str, Identifier],
schema: Union[Schema, "pa.Schema"],
location: Optional[str] = None,
partition_spec: PartitionSpec = UNPARTITIONED_PARTITION_SPEC,
sort_order: SortOrder = UNSORTED_SORT_ORDER,
properties: Properties = EMPTY_DICT,
) -> Table:
...
if isinstance(schema, pa.Schema):
schema = pre_order_visit_pyarrow(schema, _ConvertToIcebergWithFreshIds())
...
# existing code
```
We will call the function like:
```
table: pa.Table
catalog = load_catalog()
catalog.create_table('some.table', schema=table.schema)
```
And use the previously proposed Visitor: https://github.com/syun64/iceberg-python/blob/preorder-fresh-schema/pyiceberg/io/pyarrow.py#L994 since [new_table_metadata](https://github.com/apache/iceberg-python/blob/46b25be424d1ef0f28778b0873c4d91bf117a2a7/pyiceberg/table/metadata.py#L399) has to take `field_id`ed Iceberg Schema as the input.
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468546304
##########
mkdocs/docs/api.md:
##########
@@ -146,6 +146,26 @@ catalog.create_table(
)
```
+One can also create an Iceberg table using a pyarrow schema:
Review Comment:
nit:
to align with the statement above
```suggestion
To create a table using a pyarrow schema:
```
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1466705205
##########
pyiceberg/catalog/dynamodb.py:
##########
@@ -152,6 +156,14 @@ def create_table(
ValueError: If the identifier is invalid, or no path is given to store metadata.
"""
+ if not isinstance(schema, Schema):
+ import pyarrow as pa
+
+ from pyiceberg.io.pyarrow import _ConvertToIcebergWithFreshIds, pre_order_visit_pyarrow
+
+ if isinstance(schema, pa.Schema):
+ schema: Schema = pre_order_visit_pyarrow(schema, _ConvertToIcebergWithFreshIds()) # type: ignore
Review Comment:
Alright, maybe then a function in `catalog/__init__.py` that will just do the check:
```python
def _convert_schema_if_needed(schema: Union[Schema, "pa.Schema"]) -> Schema:
try:
import pa
# Do the conversion with all the imports
except ModuleNotFoundError:
pass
if not isinstance(schema, Schema):
raise ValueError(..)
return schema
```
This will reduce the code duplication and hide the awkward import logic :)
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1466700679
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Sounds great. Let me refactor that component :)
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1466736215
##########
pyiceberg/catalog/dynamodb.py:
##########
@@ -152,6 +156,14 @@ def create_table(
ValueError: If the identifier is invalid, or no path is given to store metadata.
"""
+ if not isinstance(schema, Schema):
+ import pyarrow as pa
+
+ from pyiceberg.io.pyarrow import _ConvertToIcebergWithFreshIds, pre_order_visit_pyarrow
+
+ if isinstance(schema, pa.Schema):
+ schema: Schema = pre_order_visit_pyarrow(schema, _ConvertToIcebergWithFreshIds()) # type: ignore
Review Comment:
Makes sense @Fokko π₯
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1467303335
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Thanks @syun64 for the summarization. I fully agree with your reasoning. I raised that question just to make sure we all agree that the minimum requirement for the new visitor here is to assign **arbitrarily unique IDs** for each field. Based on our discussions, there are the following options:
1. update `_ConvertToIceberg` to generate some arbitrarily unique IDs for each field in post-order
2. Add a `_ConvertToIcebergWithFreshIds` post-order visitor
3. Add a `_ConvertToIcebergWithFreshIds` pre-order visitor + `pre_order_visit_pyarrow`
Option 1 has almost no code duplication and requires slight changes only.
Option 2 requires some code duplication, which can be reduced by re-using `primitive()`(the current implementation) or reduced further by implementing some sort of inheritance as https://github.com/apache/iceberg-python/pull/219#discussion_r1456922888.
Option 3 requires some code duplication, which can be reduced by re-using `primitive()` (the current implementation)
Since the schema generated by option 3 will still have some difference in field ids assignment than that by `assign_fresh_schema_ids`, I am more inclined toward option 1 and 2. I think we might not need the `pre_order_visit_pyarrow` and `PreOrderPyArrowSchemaVisitor`.
For option 1 and 2, I have no strong preference. If there is no further concern about mixing this special case for table creation with the visitor implementaion to read table, option 1 is optimal. If we still think it may be better to separate the logic from `_ConvertToIceberg`, considering this [concern](https://github.com/apache/iceberg-python/pull/219#issuecomment-1859273052), then option 2 can be a compromise choice. WDYT?
> Is there a reason why we decided on this approach to assign_fresh_schema_ids, over using a simple pre-order traversal to assign IDs as we see the fields? Is this because we want to accommodate for potential duplicates?
I also do not know the exact reason and context here. Similarly, Java side does something more than a simple pre-order traversal. Looks like the java implementation can be traced back to the very beginning of iceberg.
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1466683196
##########
pyiceberg/catalog/dynamodb.py:
##########
@@ -152,6 +156,14 @@ def create_table(
ValueError: If the identifier is invalid, or no path is given to store metadata.
"""
+ if not isinstance(schema, Schema):
+ import pyarrow as pa
+
+ from pyiceberg.io.pyarrow import _ConvertToIcebergWithFreshIds, pre_order_visit_pyarrow
+
+ if isinstance(schema, pa.Schema):
+ schema: Schema = pre_order_visit_pyarrow(schema, _ConvertToIcebergWithFreshIds()) # type: ignore
Review Comment:
Should we also catch the exceptions when we cannot convert the schema? Also, I think we need to add a try-except when PyArrow is not available, and we can also raise an appropriate exception.
```suggestion
try:
import pyarrow as pa
from pyiceberg.io.pyarrow import _ConvertToIcebergWithFreshIds, pre_order_visit_pyarrow
if isinstance(schema, pa.Schema):
schema: Schema = pre_order_visit_pyarrow(schema, _ConvertToIcebergWithFreshIds()) # type: ignore
except ModuleNotFoundError:
# throw an appropriate error 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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1466685651
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Ouch, I still think we don't want to duplicate this whole visitor. Maybe include @HonahX his suggestion that he mentioned earlier.
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1469711334
##########
pyiceberg/io/pyarrow.py:
##########
@@ -761,6 +761,32 @@ def primitive(self, primitive: pa.DataType) -> T:
"""Visit a primitive type."""
+class PreOrderPyArrowSchemaVisitor(Generic[T], ABC):
Review Comment:
Good catch - I missed this one. I'll remove this Visitor as its no longer in use
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468603355
##########
pyiceberg/catalog/__init__.py:
##########
@@ -512,6 +516,22 @@ def _check_for_overlap(removals: Optional[Set[str]], updates: Properties) -> Non
if overlap:
raise ValueError(f"Updates and deletes have an overlap: {overlap}")
+ @staticmethod
+ def _convert_schema_if_needed(schema: Union[Schema, "pa.Schema"]) -> Schema:
+ try:
Review Comment:
Are you suggesting the following instead?
```
if isinstance(schema, Schema):
return schema
try:
import pyarrow as pa
from pyiceberg.io.pyarrow import _ConvertToIcebergWithFreshIds, pre_order_visit_pyarrow
if isinstance(schema, pa.Schema):
schema: Schema = pre_order_visit_pyarrow(schema, _ConvertToIcebergWithFreshIds()) # type: ignore
return schema
except ModuleNotFoundError:
pass
raise ValueError(f"{type(schema)=} must be pyiceberg.schema.Schema")
```
Maybe it is better, since we avoid checking for the import if the type is already correct
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1470492242
##########
tests/catalog/test_base.py:
##########
@@ -330,6 +334,34 @@ def test_create_table(catalog: InMemoryCatalog) -> None:
assert catalog.load_table(TEST_TABLE_IDENTIFIER) == table
+@pytest.mark.parametrize(
+ "schema,expected",
+ [
+ (lazy_fixture("pyarrow_schema_simple_without_ids"), lazy_fixture("iceberg_schema_simple_no_ids")),
+ (lazy_fixture("iceberg_schema_simple"), lazy_fixture("iceberg_schema_simple")),
+ (lazy_fixture("iceberg_schema_nested"), lazy_fixture("iceberg_schema_nested")),
+ (lazy_fixture("pyarrow_schema_nested_without_ids"), lazy_fixture("iceberg_schema_nested_no_ids")),
+ ],
+)
+def test_convert_schema_if_needed(
+ schema: Union[Schema, pa.Schema],
+ expected: Schema,
+ catalog: InMemoryCatalog,
+) -> None:
+ assert expected == catalog._convert_schema_if_needed(schema)
+
+
+def test_create_table_pyarrow_schema(catalog: InMemoryCatalog, pyarrow_schema_simple_without_ids: pa.Schema) -> None:
+ table = catalog.create_table(
+ identifier=TEST_TABLE_IDENTIFIER,
+ schema=pyarrow_schema_simple_without_ids,
+ location=TEST_TABLE_LOCATION,
+ partition_spec=TEST_TABLE_PARTITION_SPEC,
+ properties=TEST_TABLE_PROPERTIES,
+ )
+ assert catalog.load_table(TEST_TABLE_IDENTIFIER) == table
+
Review Comment:
Thanks for trying 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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1470494285
##########
pyiceberg/schema.py:
##########
@@ -1221,50 +1221,57 @@ def assign_fresh_schema_ids(schema_or_type: Union[Schema, IcebergType], next_id:
class _SetFreshIDs(PreOrderSchemaVisitor[IcebergType]):
"""Traverses the schema and assigns monotonically increasing ids."""
- reserved_ids: Dict[int, int]
+ old_id_to_new_id: Dict[int, int]
Review Comment:
I tried this out, and I think we might have to leave this logic in for this iteration.
In PyIceberg, a user can create a table using IcebergSchema, instead of a PyArrow schema. This means that the user can choose to include identifier_field_ids in the IcebergSchema that they define, and the current create_table or assign_fresh_ids function calls donβt have the logic to handle this outside of _SetFreshIDs
I think it might make sense to consolidate the way we update the identifier_field_ids on create_table, update_schema, replace_table and other similar operations in a future PR.
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1467757335
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
These are all good suggestions @HonahX . Thank you for taking the time to write out the detailed response π .
I'm not too keen on assigning 'random' IDs in post-order in _ConvertToIceberg or in _ConvertToIcebergWithFreshIds because that'll make it more difficult to write out the correct 'expected' outcome for the test case of the visitor when it is assigning fresh and unique IDs. Although it is deterministic, I think it's a bit difficult to leave logic that exists only for intermediary purpose, that is difficult to understand.
> I also do not know the exact reason and context here. Similarly, Java side does something more than a simple pre-order traversal. Looks like the java implementation can be traced back to the very beginning of iceberg.
If there's no additional context, I'm taking the liberty to entertain the idea of refactoring **_SetFreshIDs** visitor by analyzing its existing functionality, which is just to:
1. assign IDs in pre-order
2. track old field_id to reserved_id mapping just to ensure that old identifier_fields are mapped to the new ones at the end
If functionality (2) is the only reason we have the requirement of the field_ids needing to be unique... how do you folks feel about the idea of refactoring **_SetFreshIDs** to remove this constraint by using a set to keep track of identifier_field_ids when we **_get_and_increment** (assign) new IDs, and instead assign all field_ids as '-1' using **_ConvertToIceberg**/**_ConvertToIcebergFreshIDs**? And then use the refactored **_SetFreshIDs** visitor to assign the IDs in order?
```
class _SetFreshIDs(PreOrderSchemaVisitor[IcebergType]):
"""Traverses the schema and assigns monotonically increasing ids."""
identifier_field_ids: Set[int]
def __init__(self, next_id_func: Optional[Callable[[], int]] = None) -> None:
self.identifier_field_ids= set()
counter = itertools.count(1)
self.next_id_func = next_id_func if next_id_func is not None else lambda: next(counter)
def _get_and_increment(self, current_id: int) -> int:
new_id = self.next_id_func()
if current_id in identifier_field_ids:
identifier_field_ids.pop(current_id)
identifier_field_ids.add(new_id)
return new_id
```
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1466788214
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Is there a reason why we decided on this approach to assign_fresh_schema_ids, over using a simple pre-order traversal to assign IDs as we see the fields? Is this because we want to accommodate for potential duplicates?
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468971880
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Thanks @syun64 for sharing the comprehensive analysis!
> assign IDs in pre-order
Just want to make sure we are on the same page, my understanding is that `_SetFreshIds` is a little different from a normal pre-order visitor because it [increment the counter for each field and then visit children](https://github.com/apache/iceberg/pull/5627#discussion_r956788070). This makes it also rely on `reserved_ids` to store new ids for fields on the same level. We want to maintain this behavior when refactoring the `_SetFreshIds`. We could probably do it like the [jave implementation](https://github.com/apache/iceberg/blob/c07f2aabc0a1d02f068ecf1514d2479c0fbdd3b0/api/src/main/java/org/apache/iceberg/types/AssignFreshIds.java#L86-L97)
> assign all field_ids as '-1' using _ConvertToIceberg/_ConvertToIcebergFreshIDs? And then use the refactored _SetFreshIDs visitor to assign the IDs in order?
Overall, this approach sounds reasonable to me if we can find an easy way to refactor the `_SetFreshIds`. @Fokko, I'd appreciate your perspective on refactoring `_SetFreshIds`. Do you any issues with this approach?
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1470106203
##########
pyiceberg/schema.py:
##########
@@ -1221,50 +1221,57 @@ def assign_fresh_schema_ids(schema_or_type: Union[Schema, IcebergType], next_id:
class _SetFreshIDs(PreOrderSchemaVisitor[IcebergType]):
"""Traverses the schema and assigns monotonically increasing ids."""
- reserved_ids: Dict[int, int]
+ old_id_to_new_id: Dict[int, int]
Review Comment:
> CREATE TABLE, which is the only operation we've supported so far doesn't need to compare any IDs against a baseSchema. In the java code, idFor function uses the existing ID from the baseSchema by searching it by name if it exists. If it doesn't exist, it assigns the new ID from the counter.
Exactly, we could just pass in an empty schema (to avoid all the null-checks).
> Instead, it returns the schema object and searches the new identifier field IDs by name. Is there a reason we decided to step away from this approach in the Python implementation?
I don't think there was an explicit choice made there. The lookup by column name is correct, we do the same when evolving the schema:
https://github.com/apache/iceberg-python/blob/a3e36838b05e4ccccac434ab394af758054b12d6/pyiceberg/table/__init__.py#L1404
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1469710203
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
> I would love to get this in with the 0.6.0 release to simplify the creation of tables.
Sounds good π I'll try my best to bring this review to a consensus in the next few days. We already have a working version, so we just need to agree on the final details π
>That's an okay approach, as long as the visitor to do this is hidden inside the package. We should not expose setting -1 field IDs to the outside.
>
> What I like about the current implementation is that the visitor can be used on its own. Converting a field with all -1 IDs doesn't provide much value on its own.
I'm π with both approach, but I have a preference for the '-1' approach... and here's my current thought.
Both approaches will work, and in both approaches, we are introducing an intermediate state of the Iceberg Schema that should never be exposed. But what I like about this approach is that we are creating an intermediate Iceberg Schema that is very visibly wrong, that we could use to check the validity of the schema before we allow the erroneous intermediate state to materialize a lasting result. An example could be to introduce a new sanity check before writing the individual files, or before making a schema update, that checks that no two IDs are the same, or that the IDs are greater than 0.
Hence, I think the fact that the intermediate Iceberg Schema is "seemingly" correct actually feels more like a downside to me...
I'm curious to hear if that argument resonates with you or the rest of the group. Hopefully it will be the last discussion item before we settle on the final approach ππ
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468631158
##########
pyiceberg/catalog/__init__.py:
##########
@@ -512,6 +516,22 @@ def _check_for_overlap(removals: Optional[Set[str]], updates: Properties) -> Non
if overlap:
raise ValueError(f"Updates and deletes have an overlap: {overlap}")
+ @staticmethod
+ def _convert_schema_if_needed(schema: Union[Schema, "pa.Schema"]) -> Schema:
+ try:
Review Comment:
Yep, that's what I meant
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468964637
##########
tests/catalog/test_base.py:
##########
@@ -330,6 +333,34 @@ def test_create_table(catalog: InMemoryCatalog) -> None:
assert catalog.load_table(TEST_TABLE_IDENTIFIER) == table
+@pytest.mark.parametrize(
+ "schema_fixture,expected_fixture",
+ [
+ ("pyarrow_schema_simple_without_ids", "iceberg_schema_simple"),
+ ("iceberg_schema_simple", "iceberg_schema_simple"),
+ ("iceberg_schema_nested", "iceberg_schema_nested"),
+ ("pyarrow_schema_nested_without_ids", "iceberg_schema_nested"),
+ ],
Review Comment:
Is it possible to take advantage of `pytest_lazyfixture` here?
Like
https://github.com/apache/iceberg-python/blob/46d668b403ce94e38e63a44bd56d6f2419cf3d6d/tests/catalog/test_sql.py#L128-L135
##########
tests/catalog/test_base.py:
##########
@@ -330,6 +333,34 @@ def test_create_table(catalog: InMemoryCatalog) -> None:
assert catalog.load_table(TEST_TABLE_IDENTIFIER) == table
+@pytest.mark.parametrize(
+ "schema_fixture,expected_fixture",
+ [
+ ("pyarrow_schema_simple_without_ids", "iceberg_schema_simple"),
+ ("iceberg_schema_simple", "iceberg_schema_simple"),
+ ("iceberg_schema_nested", "iceberg_schema_nested"),
+ ("pyarrow_schema_nested_without_ids", "iceberg_schema_nested"),
+ ],
+)
+def test_convert_schema_if_needed(
+ schema_fixture: str, expected_fixture: str, catalog: InMemoryCatalog, request: pytest.FixtureRequest
+) -> None:
+ schema = request.getfixturevalue(schema_fixture)
+ expected = request.getfixturevalue(expected_fixture)
+ assert expected == catalog._convert_schema_if_needed(schema)
+
+
+def test_create_table_pyarrow_schema(catalog: InMemoryCatalog, pyarrow_schema_simple_without_ids: pa.Schema) -> None:
Review Comment:
How about combining this with the above test, so that we only need to verify if the table contain the expected schema:
```python
table = catalog.create_table(identifier..., schema=pyarrow_schema)
assert table.schema() == expected_schema
```
I think it is better to test the API directly instead of the internal method when possible. This might also resolve your concern about creating expected outcome for the "random" id assigned by whatever approach we finally choose to implement the `_convert_schema_if_needed`
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468964393
##########
tests/catalog/test_base.py:
##########
@@ -330,6 +333,34 @@ def test_create_table(catalog: InMemoryCatalog) -> None:
assert catalog.load_table(TEST_TABLE_IDENTIFIER) == table
+@pytest.mark.parametrize(
+ "schema_fixture,expected_fixture",
+ [
+ ("pyarrow_schema_simple_without_ids", "iceberg_schema_simple"),
+ ("iceberg_schema_simple", "iceberg_schema_simple"),
+ ("iceberg_schema_nested", "iceberg_schema_nested"),
+ ("pyarrow_schema_nested_without_ids", "iceberg_schema_nested"),
+ ],
+)
+def test_convert_schema_if_needed(
+ schema_fixture: str, expected_fixture: str, catalog: InMemoryCatalog, request: pytest.FixtureRequest
+) -> None:
+ schema = request.getfixturevalue(schema_fixture)
+ expected = request.getfixturevalue(expected_fixture)
+ assert expected == catalog._convert_schema_if_needed(schema)
+
+
+def test_create_table_pyarrow_schema(catalog: InMemoryCatalog, pyarrow_schema_simple_without_ids: pa.Schema) -> None:
Review Comment:
How about combining this with the above test, so that we only need to verify if the created table contain the expected schema:
```python
table = catalog.create_table(identifier..., schema=pyarrow_schema)
assert table.schema() == expected_schema
```
I think it is better to test the API directly instead of the internal method when possible. This might also resolve your concern about creating expected outcome for the "random" id assigned by whatever approach we finally choose to implement the `_convert_schema_if_needed`. WDYT?
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1466763472
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Since now we put everything behind the `Catalog.create_table`, does the visitor still need to be pre-order? The `new_table_metadata` will re-assign ids via a pre-order visitor later in the create_table workflow
https://github.com/apache/iceberg-python/blob/8464d713d69e6e2f4fa6f629362a27c940a097e6/pyiceberg/table/metadata.py#L399-L409
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Since now we put everything behind the `Catalog.create_table` API, does the visitor still need to be pre-order? The `new_table_metadata` will re-assign ids via a pre-order visitor later in the create_table workflow
https://github.com/apache/iceberg-python/blob/8464d713d69e6e2f4fa6f629362a27c940a097e6/pyiceberg/table/metadata.py#L399-L409
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468589185
##########
tests/catalog/test_base.py:
##########
@@ -255,6 +258,10 @@ def catalog() -> InMemoryCatalog:
return InMemoryCatalog("test.in.memory.catalog", **{"test.key": "test.value"})
+TEST_PYARROW_SCHEMA = pa.schema([
Review Comment:
You are right! Thank you for the nit
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1466689445
##########
pyiceberg/catalog/dynamodb.py:
##########
@@ -152,6 +156,14 @@ def create_table(
ValueError: If the identifier is invalid, or no path is given to store metadata.
"""
+ if not isinstance(schema, Schema):
+ import pyarrow as pa
+
+ from pyiceberg.io.pyarrow import _ConvertToIcebergWithFreshIds, pre_order_visit_pyarrow
+
+ if isinstance(schema, pa.Schema):
+ schema: Schema = pre_order_visit_pyarrow(schema, _ConvertToIcebergWithFreshIds()) # type: ignore
Review Comment:
I thought about that as well, but I think the main verbiage of this code block is in avoiding pyarrow imports. If we put this block of code in pyarrow.py, I think we end up importing pyarrow, or we would end up with another:
```
if not isinstance(schema, Schema):
try:
import pyarrow as pa
from pyiceberg.io.pyarrow import _new_schema_from_pyarrow_schema
if isintance(schema, pa.Schema):
schema: Schema = _new_schema_from_pyarrow_schema(schema)
...
```
So if we wanted this code block to be defined as a function, we would have to put it in a separate module where the base module imports don't include pyarrow
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468547838
##########
tests/io/test_pyarrow_visitor.py:
##########
@@ -572,3 +477,15 @@ def test_pyarrow_schema_to_schema_missing_ids_using_name_mapping_nested_missing_
with pytest.raises(ValueError) as exc_info:
_ = pyarrow_to_schema(schema, name_mapping)
assert "Could not find field with name: quux.value.key" in str(exc_info.value)
+
+
+def test_pyarrow_schema_to_schema_fresh_ids_simple_schema(
+ pyarrow_schema_simple_without_ids: pa.Schema, iceberg_schema_simple: Schema
+) -> None:
+ assert pre_order_visit_pyarrow(pyarrow_schema_simple_without_ids, _ConvertToIcebergWithFreshIds()) == iceberg_schema_simple
Review Comment:
is there a need here to check for `schema_id` equality? `Schema.__eq__` does not check for `schema_id`. `schema_id` should be `0` here
##########
tests/catalog/test_base.py:
##########
@@ -255,6 +258,10 @@ def catalog() -> InMemoryCatalog:
return InMemoryCatalog("test.in.memory.catalog", **{"test.key": "test.value"})
+TEST_PYARROW_SCHEMA = pa.schema([
Review Comment:
nit: i think this is no longer used, tests are using `pyarrow_schema_simple_without_ids` from conftest instead
##########
mkdocs/docs/api.md:
##########
@@ -146,6 +146,26 @@ catalog.create_table(
)
```
+One can also create an Iceberg table using a pyarrow schema:
+
+```python
+import pyarrow as pa
+
+pa.schema(
+ [
+ pa.field("foo", pa.string(), nullable=True),
+ pa.field("bar", pa.int32(), nullable=False),
+ pa.field("baz", pa.bool_(), nullable=True),
+ ]
+)
+
+catalog.create_table(
+ identifier="docs_example.bids",
+ schema=schema,
+ location="s3://pyiceberg",
+)
Review Comment:
```suggestion
schema = pa.schema(
[
pa.field("foo", pa.string(), nullable=True),
pa.field("bar", pa.int32(), nullable=False),
pa.field("baz", pa.bool_(), nullable=True),
]
)
catalog.create_table(
identifier="docs_example.bids",
schema=schema,
)
```
##########
pyiceberg/catalog/__init__.py:
##########
@@ -512,6 +516,22 @@ def _check_for_overlap(removals: Optional[Set[str]], updates: Properties) -> Non
if overlap:
raise ValueError(f"Updates and deletes have an overlap: {overlap}")
+ @staticmethod
+ def _convert_schema_if_needed(schema: Union[Schema, "pa.Schema"]) -> Schema:
+ try:
Review Comment:
nit: it might be easier to first type check `Schema`, then `pa.Schema`, else ValueError
##########
mkdocs/docs/api.md:
##########
@@ -146,6 +146,26 @@ catalog.create_table(
)
```
+One can also create an Iceberg table using a pyarrow schema:
Review Comment:
nit:
```suggestion
To create a table using a pyarrow schema:
```
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1470488890
##########
tests/catalog/test_base.py:
##########
@@ -330,6 +334,34 @@ def test_create_table(catalog: InMemoryCatalog) -> None:
assert catalog.load_table(TEST_TABLE_IDENTIFIER) == table
+@pytest.mark.parametrize(
+ "schema,expected",
+ [
+ (lazy_fixture("pyarrow_schema_simple_without_ids"), lazy_fixture("iceberg_schema_simple_no_ids")),
+ (lazy_fixture("iceberg_schema_simple"), lazy_fixture("iceberg_schema_simple")),
+ (lazy_fixture("iceberg_schema_nested"), lazy_fixture("iceberg_schema_nested")),
+ (lazy_fixture("pyarrow_schema_nested_without_ids"), lazy_fixture("iceberg_schema_nested_no_ids")),
+ ],
+)
+def test_convert_schema_if_needed(
+ schema: Union[Schema, pa.Schema],
+ expected: Schema,
+ catalog: InMemoryCatalog,
+) -> None:
+ assert expected == catalog._convert_schema_if_needed(schema)
+
+
+def test_create_table_pyarrow_schema(catalog: InMemoryCatalog, pyarrow_schema_simple_without_ids: pa.Schema) -> None:
+ table = catalog.create_table(
+ identifier=TEST_TABLE_IDENTIFIER,
+ schema=pyarrow_schema_simple_without_ids,
+ location=TEST_TABLE_LOCATION,
+ partition_spec=TEST_TABLE_PARTITION_SPEC,
+ properties=TEST_TABLE_PROPERTIES,
+ )
+ assert catalog.load_table(TEST_TABLE_IDENTIFIER) == table
+
Review Comment:
Thank you for the great suggestion @HonahX .
I just tried to update the test in test_base.py, but it requires us to use new_table_metadata instead of the existing mechanism of creating a new TableMetadataV1 instance, which creates a new uuid and breaks the cli/test_console.py tests.
So I think it might be simpler to fix this once we get https://github.com/apache/iceberg-python/pull/289 merged in
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1470494285
##########
pyiceberg/schema.py:
##########
@@ -1221,50 +1221,57 @@ def assign_fresh_schema_ids(schema_or_type: Union[Schema, IcebergType], next_id:
class _SetFreshIDs(PreOrderSchemaVisitor[IcebergType]):
"""Traverses the schema and assigns monotonically increasing ids."""
- reserved_ids: Dict[int, int]
+ old_id_to_new_id: Dict[int, int]
Review Comment:
I tried this out, and I think we might have to leave this logic in for this iteration.
The usage of create_table in PyIceberg is slightly different from the Java implementation in that the user can create an IcebergSchema, instead of a Spark DF schema or a PyArrow schema. This means that the user can choose to include identifier_field_ids in the IcebergSchema that they define.
I think it might make sense to consolidate the way we track the identifier_field_ids on create_table, update_schema, replace_table and other similar operations in a future PR. I'll create an issue to track this task
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1469288671
##########
pyiceberg/io/pyarrow.py:
##########
@@ -761,6 +761,32 @@ def primitive(self, primitive: pa.DataType) -> T:
"""Visit a primitive type."""
+class PreOrderPyArrowSchemaVisitor(Generic[T], ABC):
Review Comment:
```suggestion
class _PreOrderPyArrowSchemaVisitor(Generic[T], ABC):
```
##########
pyiceberg/schema.py:
##########
@@ -1221,50 +1221,57 @@ def assign_fresh_schema_ids(schema_or_type: Union[Schema, IcebergType], next_id:
class _SetFreshIDs(PreOrderSchemaVisitor[IcebergType]):
"""Traverses the schema and assigns monotonically increasing ids."""
- reserved_ids: Dict[int, int]
+ old_id_to_new_id: Dict[int, int]
def __init__(self, next_id_func: Optional[Callable[[], int]] = None) -> None:
- self.reserved_ids = {}
+ self.old_id_to_new_id = {}
counter = itertools.count(1)
self.next_id_func = next_id_func if next_id_func is not None else lambda: next(counter)
- def _get_and_increment(self) -> int:
- return self.next_id_func()
+ def _get_and_increment(self, current_id: int) -> int:
+ new_id = self.next_id_func()
+ self.old_id_to_new_id[current_id] = new_id
+ return new_id
def schema(self, schema: Schema, struct_result: Callable[[], StructType]) -> Schema:
- # First we keep the original identifier_field_ids here, we remap afterwards
- fields = struct_result().fields
- return Schema(*fields, identifier_field_ids=[self.reserved_ids[field_id] for field_id in schema.identifier_field_ids])
+ return Schema(
+ *struct_result().fields,
+ identifier_field_ids=[self.old_id_to_new_id[field_id] for field_id in schema.identifier_field_ids],
+ )
def struct(self, struct: StructType, field_results: List[Callable[[], IcebergType]]) -> StructType:
- # assign IDs for this struct's fields first
- self.reserved_ids.update({field.field_id: self._get_and_increment() for field in struct.fields})
- return StructType(*[field() for field in field_results])
+ new_ids = [self._get_and_increment(field.field_id) for field in struct.fields]
+ new_fields = []
+ for field_id, field, field_type in zip(new_ids, struct.fields, field_results):
+ new_fields.append(
+ NestedField(
+ field_id=field_id,
+ name=field.name,
+ field_type=field_type(),
+ required=field.required,
+ doc=field.doc,
+ )
Review Comment:
I think it is cleaner to keep this part in the `field` method, since it creates a field, and the `field()` now doesn't return a field, but a type.
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +932,21 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithNoIds(_ConvertToIceberg):
Review Comment:
Style suggestion, feel free to ignore:
```suggestion
class _ConvertToIcebergWithoutIDs(_ConvertToIceberg):
```
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
I also noticed that the Python implementation differs from the Java side. I don't know what the historical reason for this is. I couldn't find anything on the original PR about why this was done that way https://github.com/apache/iceberg/pull/5627. I'm okay with aligning this with the Java implementation.
> Overall, this approach sounds reasonable to me if we can find an easy way to refactor the _SetFreshIds. @Fokko, I'd appreciate your perspective on refactoring _SetFreshIds. Do you see any issues with this approach?
That's an okay approach, as long as the visitor to do this is hidden inside the package. We should not expose setting `-1` field IDs to the outside.
What I like about the current implementation is that the visitor can be used on its own. Converting a field with all `-1` IDs doesn't provide much value on its own.
I would love to get this in with the 0.6.0 release to simplify the creation of tables.
##########
pyiceberg/schema.py:
##########
@@ -1221,50 +1221,57 @@ def assign_fresh_schema_ids(schema_or_type: Union[Schema, IcebergType], next_id:
class _SetFreshIDs(PreOrderSchemaVisitor[IcebergType]):
"""Traverses the schema and assigns monotonically increasing ids."""
- reserved_ids: Dict[int, int]
+ old_id_to_new_id: Dict[int, int]
Review Comment:
I think at some point we just want to align this with Java. But let's do that in a separate PR: https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/types/AssignFreshIds.java
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1470130736
##########
pyiceberg/schema.py:
##########
@@ -1221,50 +1221,57 @@ def assign_fresh_schema_ids(schema_or_type: Union[Schema, IcebergType], next_id:
class _SetFreshIDs(PreOrderSchemaVisitor[IcebergType]):
"""Traverses the schema and assigns monotonically increasing ids."""
- reserved_ids: Dict[int, int]
+ old_id_to_new_id: Dict[int, int]
Review Comment:
Ah interesting! Does this mean we could remove this from _SetFieldIDs, and instead handle the identifier_field_names update in the corresponding operation that uses it? That would simplify this visitor even more π
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1466694858
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
:) I remember @HonahX 's suggestion, but I'm not sure if we were able to find a way to avoid code duplication. In either case, we need to create a new pre-order visitor that assigns IDs... currently, all visitors are handled post-order.
Please let me know if I missed anything we've discussed!
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468986213
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Thank you for your response! I'll give this a try and see how it turns out
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#issuecomment-1910658396
Could you also add a section to the documentation? Would be a pity if people aren't able to find this feature :)
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1466763472
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Since now we put everything behind the `Catalog.create_table` API, does the new visitor still need to be pre-order? The `new_table_metadata` will re-assign ids via a pre-order visitor later in the create_table workflow
https://github.com/apache/iceberg-python/blob/8464d713d69e6e2f4fa6f629362a27c940a097e6/pyiceberg/table/metadata.py#L399-L409
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1467303335
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Thanks @syun64 for the summarization. I fully agree with your reasoning. I raised that question just to make sure we all agree that the minimum requirement for the new visitor here is to assign **arbitrarily unique IDs** for each field. Based on our discussions, there are the following options:
1. update `_ConvertToIceberg` to generate some arbitrarily unique IDs for each field in post-order
2. Add a `_ConvertToIcebergWithFreshIds` post-order visitor
3. Add a `_ConvertToIcebergWithFreshIds` pre-order visitor + `pre_order_visit_pyarrow`
Option 1 has almost no code duplication and requires slight changes only.
Option 2 requires some code duplication, which can be reduced by re-using `primitive()`(the current implementation) or reduced further by implementing some sort of inheritance as https://github.com/apache/iceberg-python/pull/219#discussion_r1456922888.
Option 3 requires some code duplication, which can be reduced by re-using `primitive()` (the current implementation)
Since the schema generated by option 3 will still have some difference in field ids assignment than that by `assign_fresh_schema_ids`, I am more inclined toward option 1 and 2. I think we might not need the `pre_order_visit_pyarrow` and `PreOrderPyArrowSchemaVisitor`.
For option 1 and 2, I have no strong preference. If there is no further concern about mixing this special case for table creation with the visitor implementaion to read table, option 1 is optimal. If we still think it may be better to separate the logic from `_ConvertToIceberg`, considering this [concern](https://github.com/apache/iceberg-python/pull/219#issuecomment-1859273052), then option 2 can be a compromise choice. WDYT?
> Is there a reason why we decided on this approach to assign_fresh_schema_ids, over using a simple pre-order traversal to assign IDs as we see the fields? Is this because we want to accommodate for potential duplicates?
I also do not know the reason and context here. Similarly, Java side does something more than a simple pre-order traversal. Looks like the java implementation can be traced back to the very beginning of iceberg.
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1467303335
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Thanks @syun64 for the summarization. I fully agree with your reasoning. I raised that question just to make sure we all agree that the minimum requirement for the new visitor here is to assign **arbitrarily unique IDs** for each field. Based on our discussions, there are the following options:
1. update `_ConvertToIceberg` to generate some arbitrarily unique IDs for each field in post-order
2. Add a `_ConvertToIcebergWithFreshIds` post-order visitor
3. Add a `_ConvertToIcebergWithFreshIds` pre-order visitor + `pre_order_visit_pyarrow`
Option 1 has almost no code duplication and requires slight changes only.
Option 2 requires some code duplication, which can be reduced by re-using `primitive()`(the current implementation) or reduced further by implementing some sort of inheritance as https://github.com/apache/iceberg-python/pull/219#discussion_r1456922888.
Option 3 requires some code duplication, which can be reduced by re-using `primitive()` (the current implementation)
Since the schema generated by option 3 will still have some difference in field ids assignment than that by `assign_fresh_schema_ids`, I am more inclined toward option 1 and 2. I think we might not need the `pre_order_visit_pyarrow` and `PreOrderPyArrowSchemaVisitor`.
For option 1 and 2, I have no strong preference. If there is no further concern about mixing this special case for table creation with the visitor implementaion to read table, option 1 is optimal. If we still think it may be better to separate the logic from `_ConvertToIceberg`, considering this [concern](https://github.com/apache/iceberg-python/pull/219#issuecomment-1859273052), then option 2 can be a compromise choice. WDYT?
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1469706761
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +932,21 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithNoIds(_ConvertToIceberg):
Review Comment:
Thank you for the suggestion :)
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468632826
##########
pyiceberg/catalog/__init__.py:
##########
@@ -512,6 +516,22 @@ def _check_for_overlap(removals: Optional[Set[str]], updates: Properties) -> Non
if overlap:
raise ValueError(f"Updates and deletes have an overlap: {overlap}")
+ @staticmethod
+ def _convert_schema_if_needed(schema: Union[Schema, "pa.Schema"]) -> Schema:
+ try:
Review Comment:
Sounds good. These are fantastic nits, thank you! :)
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1470463965
##########
tests/catalog/test_base.py:
##########
@@ -330,6 +334,34 @@ def test_create_table(catalog: InMemoryCatalog) -> None:
assert catalog.load_table(TEST_TABLE_IDENTIFIER) == table
+@pytest.mark.parametrize(
+ "schema,expected",
+ [
+ (lazy_fixture("pyarrow_schema_simple_without_ids"), lazy_fixture("iceberg_schema_simple_no_ids")),
+ (lazy_fixture("iceberg_schema_simple"), lazy_fixture("iceberg_schema_simple")),
+ (lazy_fixture("iceberg_schema_nested"), lazy_fixture("iceberg_schema_nested")),
+ (lazy_fixture("pyarrow_schema_nested_without_ids"), lazy_fixture("iceberg_schema_nested_no_ids")),
+ ],
+)
+def test_convert_schema_if_needed(
+ schema: Union[Schema, pa.Schema],
+ expected: Schema,
+ catalog: InMemoryCatalog,
+) -> None:
+ assert expected == catalog._convert_schema_if_needed(schema)
+
+
+def test_create_table_pyarrow_schema(catalog: InMemoryCatalog, pyarrow_schema_simple_without_ids: pa.Schema) -> None:
+ table = catalog.create_table(
+ identifier=TEST_TABLE_IDENTIFIER,
+ schema=pyarrow_schema_simple_without_ids,
+ location=TEST_TABLE_LOCATION,
+ partition_spec=TEST_TABLE_PARTITION_SPEC,
+ properties=TEST_TABLE_PROPERTIES,
+ )
+ assert catalog.load_table(TEST_TABLE_IDENTIFIER) == table
+
Review Comment:
I think it may be better to have another assertion here:
```
assert table.schema() == iceberg_schema_simple
```
maybe also add fixture parametrization to include the `pyarrow_schema_nested`
This is not a big deal. So probably we can discuss/modify this in some follow-up PRs.
Currently, `test_sql.py` has covered this case:
https://github.com/apache/iceberg-python/pull/305/files#diff-fad2e9bc49f619f03bf0631fce12da1f524c786008dbdfc01a35f6caf19a7c01R161
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1470494285
##########
pyiceberg/schema.py:
##########
@@ -1221,50 +1221,57 @@ def assign_fresh_schema_ids(schema_or_type: Union[Schema, IcebergType], next_id:
class _SetFreshIDs(PreOrderSchemaVisitor[IcebergType]):
"""Traverses the schema and assigns monotonically increasing ids."""
- reserved_ids: Dict[int, int]
+ old_id_to_new_id: Dict[int, int]
Review Comment:
I tried this out, and I think we might have to leave this logic in for this iteration.
The usage of create_table in PyIceberg is slightly different from the Java implementation in that the user can create an IcebergSchema, instead of a Spark DF schema or a PyArrow schema. This means that the user can choose to include identifier_field_ids in the IcebergSchema that they define.
I think it might make sense to consolidate the way we update the identifier_field_ids on create_table, update_schema, replace_table and other similar operations in a future PR.
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#issuecomment-1915934692
Thanks @syun64 for the great work! Thanks @Fokko and @kevinjqliu for reviewing!
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1466705205
##########
pyiceberg/catalog/dynamodb.py:
##########
@@ -152,6 +156,14 @@ def create_table(
ValueError: If the identifier is invalid, or no path is given to store metadata.
"""
+ if not isinstance(schema, Schema):
+ import pyarrow as pa
+
+ from pyiceberg.io.pyarrow import _ConvertToIcebergWithFreshIds, pre_order_visit_pyarrow
+
+ if isinstance(schema, pa.Schema):
+ schema: Schema = pre_order_visit_pyarrow(schema, _ConvertToIcebergWithFreshIds()) # type: ignore
Review Comment:
Alright, maybe then a function in `catalog/__init__.py` that will just do the check:
```python
def _convert_schema_if_needed(schema: Union[Schema, "pa.Schema"]) -> Schema:
try:
import pa
# Do the conversion with all the imports
except ModuleNotFoundError:
pass
if not isinstance(schema, Schema):
raise ValueError(..)
return schema
```
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1466683944
##########
pyiceberg/catalog/dynamodb.py:
##########
@@ -152,6 +156,14 @@ def create_table(
ValueError: If the identifier is invalid, or no path is given to store metadata.
"""
+ if not isinstance(schema, Schema):
+ import pyarrow as pa
+
+ from pyiceberg.io.pyarrow import _ConvertToIcebergWithFreshIds, pre_order_visit_pyarrow
+
+ if isinstance(schema, pa.Schema):
+ schema: Schema = pre_order_visit_pyarrow(schema, _ConvertToIcebergWithFreshIds()) # type: ignore
Review Comment:
Maybe we should also move this to a helper function in `pyarrow.py`?
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1466774496
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Here's my understanding so far (please let me know if I overlooked anything):
- new_table_metadata requires a Schema
- Right now, a Schema cannot be created without field_ids assigned
- assign_fresh_schema_ids / [_SetFreshIDs](https://github.com/apache/iceberg-python/blob/0f08806d4431d5d60998dac1bca5780b6d2e2785/pyiceberg/schema.py#L1221) requires a unique ID per field to freshly assign the IDs, so we can't use a hack like assigning -1 for all the IDs, and then relying on assign_fresh_schema_ids to yield the correct result
I think the alternative would be to update **_ConvertToIceberg** to generate some arbitrarily unique IDs for each field in post-order and then relying on new_table_metadata to correctly assign the IDs from last_update_id
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468964637
##########
tests/catalog/test_base.py:
##########
@@ -330,6 +333,34 @@ def test_create_table(catalog: InMemoryCatalog) -> None:
assert catalog.load_table(TEST_TABLE_IDENTIFIER) == table
+@pytest.mark.parametrize(
+ "schema_fixture,expected_fixture",
+ [
+ ("pyarrow_schema_simple_without_ids", "iceberg_schema_simple"),
+ ("iceberg_schema_simple", "iceberg_schema_simple"),
+ ("iceberg_schema_nested", "iceberg_schema_nested"),
+ ("pyarrow_schema_nested_without_ids", "iceberg_schema_nested"),
+ ],
Review Comment:
Is it possible to take advantage of `pytest_lazyfixture`
Like
https://github.com/apache/iceberg-python/blob/46d668b403ce94e38e63a44bd56d6f2419cf3d6d/tests/catalog/test_sql.py#L128-L135
##########
tests/catalog/test_base.py:
##########
@@ -330,6 +333,34 @@ def test_create_table(catalog: InMemoryCatalog) -> None:
assert catalog.load_table(TEST_TABLE_IDENTIFIER) == table
+@pytest.mark.parametrize(
+ "schema_fixture,expected_fixture",
+ [
+ ("pyarrow_schema_simple_without_ids", "iceberg_schema_simple"),
+ ("iceberg_schema_simple", "iceberg_schema_simple"),
+ ("iceberg_schema_nested", "iceberg_schema_nested"),
+ ("pyarrow_schema_nested_without_ids", "iceberg_schema_nested"),
+ ],
+)
+def test_convert_schema_if_needed(
+ schema_fixture: str, expected_fixture: str, catalog: InMemoryCatalog, request: pytest.FixtureRequest
+) -> None:
+ schema = request.getfixturevalue(schema_fixture)
+ expected = request.getfixturevalue(expected_fixture)
+ assert expected == catalog._convert_schema_if_needed(schema)
+
+
+def test_create_table_pyarrow_schema(catalog: InMemoryCatalog, pyarrow_schema_simple_without_ids: pa.Schema) -> None:
Review Comment:
How about combining this with the above test, so that we only need to verify if the table contain the expected schema:
```python
table = catalog.create_table(identifier..., schema=pyarrow_schema)
assert table.schema() == expected_schema
```
I think it is better to test the API directly instead of the internal method when possible. This might also resolve your concern about creating expected outcome for the "random" id assigned by whatever approach we finally choose to implement the `_convert_schema_if_needed`. WDYT?
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#issuecomment-1914837022
Hi @Fokko - for some reason I don't see an option to respond to this specific Review comment, so I'm just replying here instead:
> I would love to get this in with the 0.6.0 release to simplify the creation of tables.
Sounds good π I'll try my best to bring this review to a consensus in the next few days. We already have a working version, so we just need to agree on the final details π
>That's an okay approach, as long as the visitor to do this is hidden inside the package. We should not expose setting -1 field IDs to the outside.
>
> What I like about the current implementation is that the visitor can be used on its own. Converting a field with all -1 IDs doesn't provide much value on its own.
I'm π with both approach, but I have a preference for the '-1' approach... and here's my current thought.
Both approaches will work, and in both approaches, we are introducing an intermediate state of the Iceberg Schema that should never be exposed. But what I like about this approach is that we are creating an intermediate Iceberg Schema that is very visibly wrong, that we could use to check the validity of the schema before we allow the erroneous intermediate state to materialize a lasting result. An example could be to introduce a new sanity check before writing the individual files, or before making a schema update, that checks that no two IDs are the same, or that the IDs are greater than 0.
Hence, I think the fact that the intermediate Iceberg Schema is "seemingly" correct actually feels more like a downside to me...
I'm curious to hear if that argument resonates with you or the rest of the group. Hopefully it will be the last discussion item before we settle on the final approach ππ
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1466788214
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Is there a reason why we decided on this approach to assign_fresh_schema_ids, over using a simple pre-order traversal to assign IDs as we see the fields? Is this because we want to accommodate for potential duplicates?
The usage of **reserved_ids** in _SetFreshIDs is what I'm referring to:
https://github.com/apache/iceberg-python/blob/0f08806d4431d5d60998dac1bca5780b6d2e2785/pyiceberg/schema.py#L1224-L1256
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1466698605
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Ah, I see. At least we could call the `primitive()` of the other visitor, so we don't have to duplicate the mapping of Iceberg to Arrow primitives, because that's the part that concerns me the most.
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1469643574
##########
pyiceberg/schema.py:
##########
@@ -1221,50 +1221,57 @@ def assign_fresh_schema_ids(schema_or_type: Union[Schema, IcebergType], next_id:
class _SetFreshIDs(PreOrderSchemaVisitor[IcebergType]):
"""Traverses the schema and assigns monotonically increasing ids."""
- reserved_ids: Dict[int, int]
+ old_id_to_new_id: Dict[int, int]
Review Comment:
That makes sense - I'm having an aha moment here, and just wanted to summarize my thoughts...
I think we could include the work to enable passing the baseSchema as an input argument to the visitor to [REPLACE TABLE](https://github.com/apache/iceberg-python/issues/281#issuecomment-1905187291) support since that's the function that requires fetching the original IDs from the baseSchema. CREATE TABLE, which is the only operation we've supported so far doesn't need to compare any IDs against a baseSchema. In the java code, [idFor](https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/types/AssignFreshIds.java#L51) function uses the existing ID from the baseSchema by searching it by name if it exists. If it doesn't exist, it assigns the new ID from the counter.
Another difference I see with AssignFreshIds in the java code is that it doesn't handle the reassignment of the identifier_field_ids within the visitor. Instead, it returns the schema object and searches the [new identifier field IDs](https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/types/TypeUtil.java#L262) by name. Is there a reason we decided to step away from this approach in the Python implementation?
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1470130736
##########
pyiceberg/schema.py:
##########
@@ -1221,50 +1221,57 @@ def assign_fresh_schema_ids(schema_or_type: Union[Schema, IcebergType], next_id:
class _SetFreshIDs(PreOrderSchemaVisitor[IcebergType]):
"""Traverses the schema and assigns monotonically increasing ids."""
- reserved_ids: Dict[int, int]
+ old_id_to_new_id: Dict[int, int]
Review Comment:
Ah interesting! Does this mean we could remove this from _SetFieldIDs, and instead handle the identifier_field_names update in the corresponding operation that requires updating identifier_field_names to the new IDs? That would simplify this visitor even more π
Right now, only create_table uses this visitor and doesn't require identifier_field_names to be updated (it'll always be an empty list for a new table)
If [UpdateSchema](https://github.com/apache/iceberg-python/blob/a3e36838b05e4ccccac434ab394af758054b12d6/pyiceberg/table/__init__.py#L1437) is already doing this within corresponding class that wraps the transaction, I think we could do the same in other operations that would need to do the same (like ReplaceTable) instead of keeping this feature within _SetFieldIDs
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#issuecomment-1915934234
Looks good to merge from my side. I left my closing comments on all the open discussions. Thank you @Fokko @kevinjqliu and @HonahX for your reviews!
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX merged PR #305:
URL: https://github.com/apache/iceberg-python/pull/305
--
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
Re: [PR] create_table with PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1467303335
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Thanks @syun64 for the summarization. I fully agree with your reasoning. I raised that question just to make sure we all agree that the minimum requirement for the new visitor here is to assign **arbitrarily unique IDs** for each field. Based on our discussions, there are the following options:
1. update `_ConvertToIceberg` to generate some arbitrarily unique IDs for each field in post-order
2. Add a `_ConvertToIcebergWithFreshIds` post-order visitor
3. Add a `_ConvertToIcebergWithFreshIds` pre-order visitor + `pre_order_visit_pyarrow`
Option 1 has almost no code duplication and requires slight changes only.
Option 2 requires some code duplication, which can be reduced by re-using `primitive()`(the current implementation) or reduced further by implementing some sort of inheritance as https://github.com/apache/iceberg-python/pull/219#discussion_r1456922888.
Option 3 requires some code duplication, which can be reduced by re-using `primitive()` (the current implementation)
Since the schema generated by option 3 will still have some difference in field ids assignment than that by `assign_fresh_schema_ids`, I am more inclined toward option 1 and 2. I think we might not need the `pre_order_visit_pyarrow` and `PreOrderPyArrowSchemaVisitor`.
For option 1 and 2, I have no strong preference. If there is no concern about mixing this special case for table creation with the visitor implementaion to read table, option 1 is optimal. If we still think it may be better to separate the logic from `_ConvertToIceberg`, considering this [concern](https://github.com/apache/iceberg-python/pull/219#issuecomment-1859273052), then option 2 can be a compromise choice. WDYT?
> Is there a reason why we decided on this approach to assign_fresh_schema_ids, over using a simple pre-order traversal to assign IDs as we see the fields? Is this because we want to accommodate for potential duplicates?
I also do not know the exact reason and context here. Similarly, Java side does something more than a simple pre-order traversal. Looks like the java implementation can be traced back to the very beginning of iceberg.
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1469014546
##########
tests/catalog/test_base.py:
##########
@@ -330,6 +333,34 @@ def test_create_table(catalog: InMemoryCatalog) -> None:
assert catalog.load_table(TEST_TABLE_IDENTIFIER) == table
+@pytest.mark.parametrize(
+ "schema_fixture,expected_fixture",
+ [
+ ("pyarrow_schema_simple_without_ids", "iceberg_schema_simple"),
+ ("iceberg_schema_simple", "iceberg_schema_simple"),
+ ("iceberg_schema_nested", "iceberg_schema_nested"),
+ ("pyarrow_schema_nested_without_ids", "iceberg_schema_nested"),
+ ],
Review Comment:
Thank you for the suggestion! :)
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468971880
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Thanks @syun64 for sharing the comprehensive analysis!
> assign IDs in pre-order
Just want to make sure we are on the same page, my understanding is that `_SetFreshIds` is a little different from a normal pre-order visitor because it [increment the counter for each field and then visit children](https://github.com/apache/iceberg/pull/5627#discussion_r956788070). This makes it also rely on `reserved_ids` to store new ids for fields on the same level. We want to maintain this behavior when refactoring the `_SetFreshIds`. When refactoring, we could probably do it like the [jave implementation](https://github.com/apache/iceberg/blob/c07f2aabc0a1d02f068ecf1514d2479c0fbdd3b0/api/src/main/java/org/apache/iceberg/types/AssignFreshIds.java#L86-L97)
> assign all field_ids as '-1' using _ConvertToIceberg/_ConvertToIcebergFreshIDs? And then use the refactored _SetFreshIDs visitor to assign the IDs in order?
Overall, this approach sounds reasonable to me if we can find an easy way to refactor the `_SetFreshIds`. @Fokko, I'd appreciate your perspective on refactoring `_SetFreshIds`. Do you see any issues with this approach?
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468585068
##########
tests/io/test_pyarrow_visitor.py:
##########
@@ -572,3 +477,15 @@ def test_pyarrow_schema_to_schema_missing_ids_using_name_mapping_nested_missing_
with pytest.raises(ValueError) as exc_info:
_ = pyarrow_to_schema(schema, name_mapping)
assert "Could not find field with name: quux.value.key" in str(exc_info.value)
+
+
+def test_pyarrow_schema_to_schema_fresh_ids_simple_schema(
+ pyarrow_schema_simple_without_ids: pa.Schema, iceberg_schema_simple: Schema
+) -> None:
+ assert pre_order_visit_pyarrow(pyarrow_schema_simple_without_ids, _ConvertToIcebergWithFreshIds()) == iceberg_schema_simple
Review Comment:
That's a sharp observation @kevinjqliu :) However, I don't think there's a need to check for schema_id equality because a new schema is generated with INITIAL_SCHEMA_ID in both _SetFreshIDs and _ConvertToIcebergWithFreshIds. There's an ongoing discussion on this PR that might affect whether or not we keep this visitor, so I'll leave this suggestion untouched until we reach a consensus there:
https://github.com/apache/iceberg-python/pull/305#discussion_r1467757335
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468585851
##########
pyiceberg/catalog/__init__.py:
##########
@@ -512,6 +516,22 @@ def _check_for_overlap(removals: Optional[Set[str]], updates: Properties) -> Non
if overlap:
raise ValueError(f"Updates and deletes have an overlap: {overlap}")
+ @staticmethod
+ def _convert_schema_if_needed(schema: Union[Schema, "pa.Schema"]) -> Schema:
+ try:
Review Comment:
This approach is a result of an earlier discussion with @Fokko : https://github.com/apache/iceberg-python/pull/305#discussion_r1466705205
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1469674294
##########
pyiceberg/schema.py:
##########
@@ -1221,50 +1221,57 @@ def assign_fresh_schema_ids(schema_or_type: Union[Schema, IcebergType], next_id:
class _SetFreshIDs(PreOrderSchemaVisitor[IcebergType]):
"""Traverses the schema and assigns monotonically increasing ids."""
- reserved_ids: Dict[int, int]
+ old_id_to_new_id: Dict[int, int]
def __init__(self, next_id_func: Optional[Callable[[], int]] = None) -> None:
- self.reserved_ids = {}
+ self.old_id_to_new_id = {}
counter = itertools.count(1)
self.next_id_func = next_id_func if next_id_func is not None else lambda: next(counter)
- def _get_and_increment(self) -> int:
- return self.next_id_func()
+ def _get_and_increment(self, current_id: int) -> int:
+ new_id = self.next_id_func()
+ self.old_id_to_new_id[current_id] = new_id
+ return new_id
def schema(self, schema: Schema, struct_result: Callable[[], StructType]) -> Schema:
- # First we keep the original identifier_field_ids here, we remap afterwards
- fields = struct_result().fields
- return Schema(*fields, identifier_field_ids=[self.reserved_ids[field_id] for field_id in schema.identifier_field_ids])
+ return Schema(
+ *struct_result().fields,
+ identifier_field_ids=[self.old_id_to_new_id[field_id] for field_id in schema.identifier_field_ids],
+ )
def struct(self, struct: StructType, field_results: List[Callable[[], IcebergType]]) -> StructType:
- # assign IDs for this struct's fields first
- self.reserved_ids.update({field.field_id: self._get_and_increment() for field in struct.fields})
- return StructType(*[field() for field in field_results])
+ new_ids = [self._get_and_increment(field.field_id) for field in struct.fields]
+ new_fields = []
+ for field_id, field, field_type in zip(new_ids, struct.fields, field_results):
+ new_fields.append(
+ NestedField(
+ field_id=field_id,
+ name=field.name,
+ field_type=field_type(),
+ required=field.required,
+ doc=field.doc,
+ )
Review Comment:
Yes, I realized that too...
I think the refactored behavior actually [mirrors the Java code](https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/types/AssignFreshIds.java#L102) more closely now in how it takes the Type, and then creates the NestedField with the result of the field level Callable within the struct function call in order to assign the IDs first to the fields, and then to its children.
I think preserving this approach allows us to avoid introducing the new constraint of requiring the field IDs to be unique, and limits that restriction to just the [correctness in the task of refreshing the identifier_field_ids](https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/types/TypeUtil.java#L262), which requires referring to the field Name by index as well as by name.
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468632375
##########
pyiceberg/catalog/__init__.py:
##########
@@ -512,6 +516,22 @@ def _check_for_overlap(removals: Optional[Set[str]], updates: Properties) -> Non
if overlap:
raise ValueError(f"Updates and deletes have an overlap: {overlap}")
+ @staticmethod
+ def _convert_schema_if_needed(schema: Union[Schema, "pa.Schema"]) -> Schema:
+ try:
Review Comment:
And also update ValueError message to include pyarrow.Schema
--
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
Re: [PR] `create_table` with a PyArrow Schema [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #305:
URL: https://github.com/apache/iceberg-python/pull/305#discussion_r1468971880
##########
pyiceberg/io/pyarrow.py:
##########
@@ -906,6 +986,76 @@ def after_map_value(self, element: pa.Field) -> None:
self._field_names.pop()
+class _ConvertToIcebergWithFreshIds(PreOrderPyArrowSchemaVisitor[Union[IcebergType, Schema]]):
Review Comment:
Thanks @syun64 for sharing the comprehensive analysis!
> assign IDs in pre-order
Just want to make sure we are on the same page, my understanding is that `_SetFreshIds` is a little different from a normal pre-order visitor because it [increment the counter for each field and then visit children](https://github.com/apache/iceberg/pull/5627#discussion_r956788070). This makes it also rely on `reserved_ids` to store new ids for fields on the same level. We want to maintain this behavior when refactoring the `_SetFreshIds`. When refactoring, we could probably do it like the [jave implementation](https://github.com/apache/iceberg/blob/c07f2aabc0a1d02f068ecf1514d2479c0fbdd3b0/api/src/main/java/org/apache/iceberg/types/AssignFreshIds.java#L86-L97)
> assign all field_ids as '-1' using _ConvertToIceberg/_ConvertToIcebergFreshIDs? And then use the refactored _SetFreshIDs visitor to assign the IDs in order?
Overall, this approach sounds reasonable to me if we can find an easy way to refactor the `_SetFreshIds`. @Fokko, I'd appreciate your perspective on refactoring `_SetFreshIds`. Do you any issues with this approach?
--
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