Uh oh!
There was an error while loading. Please reload this page.
Remove Generic from expressions - #2750
Conversation
From the beginning we had these Generic in the expressions system, but it really never worked as we hoped. It came from Java where Generics are much stronger, but the static typing of Python/mypy doesn't really follow the types.
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM! mypy is happy too
Uh oh!
There was an error while loading. Please reload this page.
sungwy
left a comment
There was a problem hiding this comment.
Thank you for the PR @Fokko !
I think @kevinjqliu beat me to the punch while I was reviewing it, but I left some comments. 😄
| @abstractmethod | ||
| def visit_equal(self, term: BoundTerm[L], literal: Literal[L]) -> T: | ||
| def visit_equal(self, term: BoundTerm, literal: Literal[L]) -> T: |
There was a problem hiding this comment.
nit, but do you think it would make sense to align with your change in expressions/__init__.py and use LiteralValue here?
| defvisit_equal(self, term: BoundTerm, literal: Literal[L]) ->T: | |
| defvisit_equal(self, term: BoundTerm, literal: LiteralValue) ->T: |
| if TYPE_CHECKING: | ||
| LiteralValue = Literal[Any] | ||
| else: | ||
| LiteralValue = Literal |
There was a problem hiding this comment.
What are your thoughts on moving this to pyiceberg.typedef instead? I think that'll allow it to be reused by other modules for static type checking.
There was a problem hiding this comment.
good one! sorry i merged it before you got a chance to submit your reviews
Looks like Fokko has followed up with #2769 :)
Rationale for this change
This is a big one, and was on my mind for a long time, but I think we should clean this up before a 1.x release.
I bumped into this once more when I tried to extend from the
IcebergBaseModel, but that did not work well with theGeneric.From the beginning we had these Generic in the expressions system, but it really never worked as we hoped. It came from Java where Generics are much stronger, but the static typing of Python/mypy doesn't really follow the types.
Are these changes tested?
Are there any user-facing changes?