Python: Allow to pass in a string as filter #6657
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Often I have to look up the exact of the operator, I think it would be nice to allow the end user to provide a string that we'll parse with the excellent parser that we already have.
amogh-jahagirdar
approved these changes
Jan 29, 2023
There was a problem hiding this comment.
Just a nit comment, overall this makes sense to me, it would be really nice to just pass in a string predicate. Thanks @Fokko !
python/pyiceberg/table/__init__.py
Outdated
Comment on lines
195
to
198
| if row_filter is None: | ||
| self.row_filter = AlwaysTrue() | ||
| else: | ||
| self.row_filter = parser.parse(row_filter) |
There was a problem hiding this comment.
Nit: Maybe we could use a ternary operator here?
self.row_filter = parser.parse(row_filter) if row_filter else AlwaysTrue()
There was a problem hiding this comment.
Good suggestion, I've split it into a separate method for now, that also takes care of the parsing.
rdblue
reviewed
Jan 31, 2023
| @@ -232,6 +233,9 @@ def handle_or(result: ParseResults) -> Or: | |||
| ).set_name("expr") | |||
|
|
|||
|
|
|||
| def parse(expr: str) -> BooleanExpression: | |||
| def parse(expr: Union[str, BooleanExpression]) -> BooleanExpression: | |||
There was a problem hiding this comment.
I can see why you'd do this, but it seems better to make the caller check before calling parse. It's a bit confusing to accept an expression here and do nothing.
rdblue
reviewed
Jan 31, 2023
rdblue
approved these changes
Feb 7, 2023
|
Thanks for the update, @Fokko! Looks good now so I merged. |
krvikash
pushed a commit
to krvikash/iceberg
that referenced
this pull request
Mar 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Often I have to look up the name of the operator, I think it would be nice to allow the end user to provide a string that we'll parse with the excellent parser that we already have.
Works both: