You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "frankliee (via GitHub)" <gi...@apache.org> on 2024/04/16 01:58:43 UTC
[PR] Tests: Unify the test catalog setting [iceberg-python]
frankliee opened a new pull request, #609:
URL: https://github.com/apache/iceberg-python/pull/609
In test_reads.py and test_partition_evolution.py, there are redundant` catalog_rest() `and `catalog_hive()` for setting the test catalog.
This PR will unify the test catalog setting in conftest.py to enhance maintainability.
--
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] Tests: Unify the test catalog setting [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX merged PR #609:
URL: https://github.com/apache/iceberg-python/pull/609
--
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] Tests: Unify the test catalog setting [iceberg-python]
Posted by "frankliee (via GitHub)" <gi...@apache.org>.
frankliee commented on code in PR #609:
URL: https://github.com/apache/iceberg-python/pull/609#discussion_r1566818851
##########
tests/conftest.py:
##########
@@ -2144,3 +2144,31 @@ def arrow_table_with_only_nulls(pa_schema: "pa.Schema") -> "pa.Table":
import pyarrow as pa
return pa.Table.from_pylist([{}, {}], schema=pa_schema)
+
+
+@pytest.fixture()
+def catalog_rest() -> Catalog:
+ return load_catalog(
+ "local",
+ **{
+ "type": "rest",
+ "uri": "http://localhost:8181",
+ "s3.endpoint": "http://localhost:9000",
+ "s3.access-key-id": "admin",
+ "s3.secret-access-key": "password",
+ },
+ )
+
+
+@pytest.fixture()
+def catalog_hive() -> Catalog:
Review Comment:
> @frankliee Thanks for working on this! We also have `session_catalog` and `session_hive_catalog` in `conftest`
>
> https://github.com/apache/iceberg-python/blob/aa4654368e54bf84933279179519b299b5910493/tests/conftest.py#L1997-L2022
>
>
> and I think they can be unified with these too (using scope `session`). WDYT?
Nice suggestion, I have unified all of them as `session_catalog` and `session_hive_catalog` in this 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] Tests: Unify the test catalog setting [iceberg-python]
Posted by "frankliee (via GitHub)" <gi...@apache.org>.
frankliee commented on code in PR #609:
URL: https://github.com/apache/iceberg-python/pull/609#discussion_r1566818851
##########
tests/conftest.py:
##########
@@ -2144,3 +2144,31 @@ def arrow_table_with_only_nulls(pa_schema: "pa.Schema") -> "pa.Table":
import pyarrow as pa
return pa.Table.from_pylist([{}, {}], schema=pa_schema)
+
+
+@pytest.fixture()
+def catalog_rest() -> Catalog:
+ return load_catalog(
+ "local",
+ **{
+ "type": "rest",
+ "uri": "http://localhost:8181",
+ "s3.endpoint": "http://localhost:9000",
+ "s3.access-key-id": "admin",
+ "s3.secret-access-key": "password",
+ },
+ )
+
+
+@pytest.fixture()
+def catalog_hive() -> Catalog:
Review Comment:
> @frankliee Thanks for working on this! We also have `session_catalog` and `session_hive_catalog` in `conftest`
>
> https://github.com/apache/iceberg-python/blob/aa4654368e54bf84933279179519b299b5910493/tests/conftest.py#L1997-L2022
>
>
> and I think they can be unified with these too (using scope `session`). WDYT?
Nice suggestion, I will fix them in this PR after a while.
--
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] Tests: Unify the test catalog setting [iceberg-python]
Posted by "frankliee (via GitHub)" <gi...@apache.org>.
frankliee commented on code in PR #609:
URL: https://github.com/apache/iceberg-python/pull/609#discussion_r1566818851
##########
tests/conftest.py:
##########
@@ -2144,3 +2144,31 @@ def arrow_table_with_only_nulls(pa_schema: "pa.Schema") -> "pa.Table":
import pyarrow as pa
return pa.Table.from_pylist([{}, {}], schema=pa_schema)
+
+
+@pytest.fixture()
+def catalog_rest() -> Catalog:
+ return load_catalog(
+ "local",
+ **{
+ "type": "rest",
+ "uri": "http://localhost:8181",
+ "s3.endpoint": "http://localhost:9000",
+ "s3.access-key-id": "admin",
+ "s3.secret-access-key": "password",
+ },
+ )
+
+
+@pytest.fixture()
+def catalog_hive() -> Catalog:
Review Comment:
> @frankliee Thanks for working on this! We also have `session_catalog` and `session_hive_catalog` in `conftest`
>
> https://github.com/apache/iceberg-python/blob/aa4654368e54bf84933279179519b299b5910493/tests/conftest.py#L1997-L2022
>
>
> and I think they can be unified with these too (using scope `session`). WDYT?
Nice suggestion, I will unify all of them as `session_catalog` and `session_hive_catalog` in this PR after a while.
--
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] Tests: Unify the test catalog setting [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #609:
URL: https://github.com/apache/iceberg-python/pull/609#discussion_r1566772517
##########
tests/conftest.py:
##########
@@ -2144,3 +2144,31 @@ def arrow_table_with_only_nulls(pa_schema: "pa.Schema") -> "pa.Table":
import pyarrow as pa
return pa.Table.from_pylist([{}, {}], schema=pa_schema)
+
+
+@pytest.fixture()
+def catalog_rest() -> Catalog:
+ return load_catalog(
+ "local",
+ **{
+ "type": "rest",
+ "uri": "http://localhost:8181",
+ "s3.endpoint": "http://localhost:9000",
+ "s3.access-key-id": "admin",
+ "s3.secret-access-key": "password",
+ },
+ )
+
+
+@pytest.fixture()
+def catalog_hive() -> Catalog:
Review Comment:
@frankliee Thanks for working on this! We also have `session_catalog` and `session_hive_catalog` in `conftest`
https://github.com/apache/iceberg-python/blob/aa4654368e54bf84933279179519b299b5910493/tests/conftest.py#L1997-L2022
and I think they can be unified with these too (using scope `session`). 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] Tests: Unify the test catalog setting [iceberg-python]
Posted by "frankliee (via GitHub)" <gi...@apache.org>.
frankliee commented on code in PR #609:
URL: https://github.com/apache/iceberg-python/pull/609#discussion_r1566818851
##########
tests/conftest.py:
##########
@@ -2144,3 +2144,31 @@ def arrow_table_with_only_nulls(pa_schema: "pa.Schema") -> "pa.Table":
import pyarrow as pa
return pa.Table.from_pylist([{}, {}], schema=pa_schema)
+
+
+@pytest.fixture()
+def catalog_rest() -> Catalog:
+ return load_catalog(
+ "local",
+ **{
+ "type": "rest",
+ "uri": "http://localhost:8181",
+ "s3.endpoint": "http://localhost:9000",
+ "s3.access-key-id": "admin",
+ "s3.secret-access-key": "password",
+ },
+ )
+
+
+@pytest.fixture()
+def catalog_hive() -> Catalog:
Review Comment:
> @frankliee Thanks for working on this! We also have `session_catalog` and `session_hive_catalog` in `conftest`
>
> https://github.com/apache/iceberg-python/blob/aa4654368e54bf84933279179519b299b5910493/tests/conftest.py#L1997-L2022
>
>
> and I think they can be unified with these too (using scope `session`). WDYT?
Nice suggestion, I will unify all of them as `catalog_rest` and `catalog_hive` in this PR after a while.
--
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