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
[email protected](
+ "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
[email protected](
+ "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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
