Skip to content

Python: Remove mypy error #4728

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rdblue merged 1 commit into from
May 10, 2022
Merged

Python: Remove mypy error #4728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 6 additions & 11 deletions python/src/iceberg/transforms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@ def __repr__(self):
def __str__(self):
return self._transform_string

def __call__(self, value: S) -> Optional[T]:
def __call__(self, value: Optional[S]) -> Optional[T]:
return self.apply(value)

def apply(self, value: S) -> Optional[T]:
def apply(self, value: Optional[S]) -> Optional[T]:
...

def can_transform(self, source: IcebergType) -> bool:
Expand All @@ -83,10 +83,8 @@ def preserves_order(self) -> bool:
def satisfies_order_of(self, other) -> bool:
return self == other

def to_human_string(self, value) -> str:
if value is None:
return "null"
return str(value)
def to_human_string(self, value: Optional[S]) -> str:
return str(value) if value is not None else "null"
Copy link
Contributor

@rdblue rdblue May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change necessary for this PR? We generally try to avoid code churn that isn't making functional changes.


@property
def dedup_name(self) -> str:
Expand Down Expand Up @@ -119,11 +117,8 @@ def num_buckets(self) -> int:
def hash(self, value: S) -> int:
raise NotImplementedError()

def apply(self, value: S) -> Optional[int]:
if value is None:
Copy link
Contributor Author

@Fokko Fokko May 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have S, it can never be None, otherwise, the signature should be Optional[S]. If we want, we can still check if it None, "" or [] using:

if not value:

I've removed it for now, because according to the current definition it is unreachable code.

Copy link
Contributor Author

@Fokko Fokko May 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, looking at the tests, we might expect a None there. I've changed the code the other way around.

Copy link
Contributor

@rdblue rdblue May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, transforms can be applied to optional columns so this should be Optional[S].

return None

return (self.hash(value) & IntegerType.max) % self._num_buckets
def apply(self, value: Optional[S]) -> Optional[int]:
return (self.hash(value) & IntegerType.max) % self._num_buckets if value else None

def result_type(self, source: IcebergType) -> IcebergType:
return IntegerType()
Expand Down
8 changes: 7 additions & 1 deletion python/tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ commands =
deps =
mypy
commands =
mypy --no-implicit-optional --install-types --non-interactive --config tox.ini src
mypy --install-types --non-interactive --config tox.ini src

[testenv:docs]
basepython = python3
Expand Down Expand Up @@ -97,5 +97,11 @@ python =
3.8: py38, linters
3.9: py39

[mypy]

no_implicit_optional=True
warn_redundant_casts=True
warn_unreachable=True

[mypy-pyarrow.*]
ignore_missing_imports = True