You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/07/18 16:58:39 UTC
[GitHub] [iceberg] rymurr opened a new pull request #1214: [Python] Minor fixes for tests
rymurr opened a new pull request #1214:
URL: https://github.com/apache/iceberg/pull/1214
In the course of implementing create table I came across minor issues:
* rename test fixtures to suppress warnings and prevent pytest from treating them as tests
* test partition code path & fix incorrect signature when creating metadata
* handle `file:` as well as `file://` as it seems both schemas are used
* add mypy annotations in areas where I fixed code
* fix tox/mypy
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] xhochy commented on a change in pull request #1214: [Python] Minor fixes for tests
Posted by GitBox <gi...@apache.org>.
xhochy commented on a change in pull request #1214:
URL: https://github.com/apache/iceberg/pull/1214#discussion_r457232658
##########
File path: python/tests/core/test_base_table_scan.py
##########
@@ -14,11 +14,12 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-
from iceberg.api import Schema
from iceberg.api.types import IntegerType, NestedField
+import pytest
+@pytest.mark.parametrize("ts_table", ['one', 'none'], indirect=True)
Review comment:
Is this still needed with the `ts_table` fixture already parametrized?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1214: [Python] Minor fixes for tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1214:
URL: https://github.com/apache/iceberg/pull/1214#discussion_r457773636
##########
File path: python/tests/core/test_base_table_scan.py
##########
@@ -14,7 +14,6 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-
Review comment:
Nit: this file doesn't need to change and could cause git conflicts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1214: [Python] Minor fixes for tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1214:
URL: https://github.com/apache/iceberg/pull/1214#issuecomment-661511527
Overall looks good. Just a couple of minor things. I'm okay to merge if you want, but I would normally not introduce changes to 3 files just for renames.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr commented on a change in pull request #1214: [Python] Minor fixes for tests
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #1214:
URL: https://github.com/apache/iceberg/pull/1214#discussion_r457859627
##########
File path: python/tests/core/test_base_table_scan.py
##########
@@ -14,7 +14,6 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-
Review comment:
agreed, fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr commented on a change in pull request #1214: [Python] Minor fixes for tests
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #1214:
URL: https://github.com/apache/iceberg/pull/1214#discussion_r457458124
##########
File path: python/tests/core/test_base_table_scan.py
##########
@@ -14,11 +14,12 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-
from iceberg.api import Schema
from iceberg.api.types import IntegerType, NestedField
+import pytest
+@pytest.mark.parametrize("ts_table", ['one', 'none'], indirect=True)
Review comment:
thanks for that. You are correct! fixed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1214: [Python] Minor fixes for tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1214:
URL: https://github.com/apache/iceberg/pull/1214#discussion_r457773039
##########
File path: python/iceberg/api/partition_spec.py
##########
@@ -183,7 +183,7 @@ def __repr__(self):
return "".join(sb)
@staticmethod
- def builder_for(schema):
+ def builder_for(schema: Schema) -> "PartitionSpecBuilder":
Review comment:
Why is `PartitionSpecBuilder` in quotes?
Not a problem, I've just not seen that before. `Schema` isn't in quotes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1214: [Python] Minor fixes for tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1214:
URL: https://github.com/apache/iceberg/pull/1214#discussion_r457773875
##########
File path: python/tests/api/test_helpers.py
##########
@@ -74,7 +74,7 @@ def predicate(self, pred):
return None
-class TestDataFile(DataFile):
+class MockDataFile(DataFile):
Review comment:
Are the renames from `Mock` to `Test` worth the trouble? Unless this is following a python convention, it seems like this is just going to cause git conflicts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr commented on pull request #1214: [Python] Minor fixes for tests
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #1214:
URL: https://github.com/apache/iceberg/pull/1214#issuecomment-660510967
This batches a few fixes together. Hope that is ok, in this case I thought it was easier than 3-4 very small changes. If there is a preference for multiple small commits I am happy to close this and separate them 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr commented on pull request #1214: [Python] Minor fixes for tests
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #1214:
URL: https://github.com/apache/iceberg/pull/1214#issuecomment-661662535
Thanks for the review @rdblue, commented and pushed some changes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #1214: [Python] Minor fixes for tests
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #1214:
URL: https://github.com/apache/iceberg/pull/1214
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1214: [Python] Minor fixes for tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1214:
URL: https://github.com/apache/iceberg/pull/1214#discussion_r458219274
##########
File path: python/tests/api/test_helpers.py
##########
@@ -74,7 +74,7 @@ def predicate(self, pred):
return None
-class TestDataFile(DataFile):
+class MockDataFile(DataFile):
Review comment:
Sounds reasonable to me.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] xhochy commented on a change in pull request #1214: [Python] Minor fixes for tests
Posted by GitBox <gi...@apache.org>.
xhochy commented on a change in pull request #1214:
URL: https://github.com/apache/iceberg/pull/1214#discussion_r457953657
##########
File path: python/tests/api/test_helpers.py
##########
@@ -74,7 +74,7 @@ def predicate(self, pred):
return None
-class TestDataFile(DataFile):
+class MockDataFile(DataFile):
Review comment:
It would be preferable to not name them `Test…` as this is the standard prefix for classes that actually have tests.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr commented on a change in pull request #1214: [Python] Minor fixes for tests
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #1214:
URL: https://github.com/apache/iceberg/pull/1214#discussion_r457865413
##########
File path: python/tests/api/test_helpers.py
##########
@@ -74,7 +74,7 @@ def predicate(self, pred):
return None
-class TestDataFile(DataFile):
+class MockDataFile(DataFile):
Review comment:
Up to you really @rdblue. I may be nit picking but I dislike the visual noise in pytest outputs. Master writes 8-10 lines of warnings in yellow and this patch writes one line in green.
The rename suppresses warnings like below. Given the way pytest aggressively searches for test patterns I think its convention to only have test in the name if you want pytest to try and test it.
``` bash
tests/api/test_helpers.py:77
/home/ryan/workspace/iceberg/python/tests/api/test_helpers.py:77: PytestCollectionWarning: cannot collect test class 'TestDataFile' because it has a __init__ constructor (from: tests/api/test_helpers.py)
class TestDataFile(DataFile):
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr commented on a change in pull request #1214: [Python] Minor fixes for tests
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #1214:
URL: https://github.com/apache/iceberg/pull/1214#discussion_r457859386
##########
File path: python/iceberg/api/partition_spec.py
##########
@@ -183,7 +183,7 @@ def __repr__(self):
return "".join(sb)
@staticmethod
- def builder_for(schema):
+ def builder_for(schema: Schema) -> "PartitionSpecBuilder":
Review comment:
Its because this is above the `PartitionSpecBuilder` class in the file. So the `PartitionSpecBuilder` class isn't yet reference-able. One of the small annoyances of mypy. Moving the class to the top of the file or to the a new file would fix it but create a needlessly larger diff
Note: the same double-quote notation is used for a method in `PartitionSpecBuilder` which returns a `PartitionSpecBuilder` for the same reason, this is unfortunately unavoidable (to my knowledge) .
----------------------------------------------------------------
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.
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