Skip to content
This repository has been archived by the owner on Nov 22, 2022. It is now read-only.

#394: Use Union[List[Column], List[str]] for Select #395

Merged
merged 2 commits into from Apr 16, 2020

Conversation

jhereth
Copy link
Contributor

@jhereth jhereth commented Apr 3, 2020

Passing a List[str] to select raises a mypy warning, similar for List[Column]. We change the type from List[Union[Column, str]] to Union[List[Column], List[str]].

Fixes #394 .

@jhereth jhereth force-pushed the qudade_394_Sequence_for_select branch 2 times, most recently from c27ee8e to bf7fcb0 Compare April 3, 2020 13:52
Copy link
Owner

@zero323 zero323 left a comment

Choose a reason for hiding this comment

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

Thanks for you work @qudade.

Unfortunately, this is not going to work. In fact Spark is very specific about types here, and the only support collection is list.

Sequence would type check for all kind of different types, like

>>> isinstance(range(10), Sequence)                                                                                                                                                      
True
>>> isinstance((), Sequence)                                                                                                                                                             
True

which are clearly not supported on runtime.

If you want to address this particular scenario you should use Union[List[Column], List[str]], i.e.

 def select(self, __cols: Union[List[Column], List[str]]) -> DataFrame: ...

though it should probably cover other cases with similar signature.

jhereth added a commit to jhereth/pyspark-stubs that referenced this pull request Apr 3, 2020
as @zero323 pointed out in zero323#395 (review),
we can use only List, not Sequence
@jhereth jhereth requested a review from zero323 April 3, 2020 21:12
@zero323
Copy link
Owner

zero323 commented Apr 4, 2020

That looks much better. Would you mind adding data tests for this?

@jhereth
Copy link
Contributor Author

jhereth commented Apr 4, 2020

@zero323 sure, I'll try to add some tests next week.

Is there some information how to run the tests? (simply calling pytest didn't seem to do the trick)

@zero323 zero323 changed the title #394: Use Sequence for Select #394: Use Union[List[Column], List[str]] for Select Apr 4, 2020
@zero323
Copy link
Owner

zero323 commented Apr 4, 2020

@zero323 sure, I'll try to add some tests next week.

Thanks.

Is there some information how to run the tests? (simply calling pytest didn't seem to do the trick)

You'll have to add annotations directory to your MYPYPATH and set MYPY_TEST_PREFIX, i.e.

- MYPY_TEST_PREFIX=$PWD MYPYPATH=$PWD/third_party/3 pytest

jhereth added a commit to jhereth/pyspark-stubs that referenced this pull request Apr 15, 2020
as @zero323 pointed out in zero323#395 (review),
we can use only List, not Sequence (as originally suggested by mypy)
@jhereth jhereth force-pushed the qudade_394_Sequence_for_select branch from b824d80 to 270a9c3 Compare April 15, 2020 20:17
as @zero323 pointed out in zero323#395 (review),
we can use only List, not Sequence (as originally suggested by mypy)
@jhereth jhereth force-pushed the qudade_394_Sequence_for_select branch from 270a9c3 to 48c60e9 Compare April 15, 2020 20:34
@jhereth
Copy link
Contributor Author

jhereth commented Apr 15, 2020

@zero323 Tests have been added. Code was rebased vs master and squashed.

Tests are passing (except for #397)

@jhereth jhereth requested a review from zero323 April 16, 2020 16:40
@zero323 zero323 merged commit fd769b9 into zero323:master Apr 16, 2020
@jhereth jhereth deleted the qudade_394_Sequence_for_select branch April 16, 2020 19:49
@zero323
Copy link
Owner

zero323 commented Apr 17, 2020

Merged to master, thank you.

zero323 added a commit that referenced this pull request Apr 28, 2020
as @zero323 pointed out in #395 (review),
we can use only List, not Sequence (as originally suggested by mypy)

Co-authored-by: Maciej <zero323@users.noreply.github.com>
zero323 added a commit that referenced this pull request Apr 28, 2020
we can use only List, not Sequence (as originally suggested by mypy)

Co-authored-by: Maciej <zero323@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use covariant Sequence for select()
2 participants