Skip to content

Documented row_filter expressions #1862

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.

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

norton120
Copy link
Contributor

Rationale for this change

scan's row_filter param is not super intuitive. I got tired of reading over the expression and parser code as I'm trying to build out statements, so I had some docs made up.

Are these changes tested?

They are docs only, so not really?

Are there any user-facing changes?

Yes there are docs for the expression and string syntaxes of row_filter now.

norton120 and others added 2 commits April 4, 2025 15:03
Co-authored-by: Fokko Driesprong <fokko@apache.org>
Co-authored-by: Fokko Driesprong <fokko@apache.org>
@norton120 norton120 requested a review from Fokko April 4, 2025 19:03
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Thanks @norton120 I think this is a great addition to the docs 🙌 I left some small suggestions, let me know what you think of it 👍

norton120 and others added 3 commits April 15, 2025 10:51
Co-authored-by: Fokko Driesprong <fokko@apache.org>
Co-authored-by: Fokko Driesprong <fokko@apache.org>
Co-authored-by: Fokko Driesprong <fokko@apache.org>
@norton120 norton120 force-pushed the string-parsing-docs branch from 5b347e4 to 21b6196 Compare April 15, 2025 14:54
@norton120
Copy link
Contributor Author

bah 🤦🏻 I clobbered the in-GH changes while rebasing main. grr.
I'll have to pick them back first and then can fix per the last open comments

@norton120
Copy link
Contributor Author

Sorry these last fixes took so long I missed the notifications. Updated!

@Fokko
Copy link
Contributor

Fokko commented Apr 15, 2025

@norton120 no worries, thanks for following up! 🙌

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

@norton120 I have two more minor issues while going over it

Comment on lines +215 to +216
```python

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
```python
```python

age_equals_18 = EqualTo("age", 18)

# This will raise a TypeError if the field type doesn't match
age_equals_18 = EqualTo("age", "18") # Will fail if age is an integer field
Copy link
Contributor

Choose a reason for hiding this comment

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

This should work:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants