Skip to content

Conversation

jtuglu1
Copy link
Contributor

@jtuglu1 jtuglu1 commented Aug 16, 2025

Rationale for this change

Fixes #792

Are these changes tested?

Are there any user-facing changes?

Add BETWEEN operator

@jtuglu1 jtuglu1 force-pushed the add-between-keyword branch 3 times, most recently from acd1b28 to ee8f952 Compare August 16, 2025 02:32
Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for adding this

@kevinjqliu
Copy link
Contributor

looks like the linter is breaking CI, can you run make lint locally?

Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

LGTM!

Could you do make lint again?

markdownlint.............................................................Failed
- hook id: markdownlint
- files were modified by this hook

And put the PR in "review" instead of draft

@jtuglu1 jtuglu1 marked this pull request as ready for review August 17, 2025 22:06
@jtuglu1
Copy link
Contributor Author

jtuglu1 commented Aug 17, 2025

make lint

Running this locally already passes everything, I'll try rebasing from main and marking ready for review:

❯ make lint
poetry run pre-commit run --all-files
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
debug statements (python)................................................Passed
check yaml...............................................................Passed
check python ast.........................................................Passed
ruff (legacy alias)......................................................Passed
ruff format..............................................................Passed
mypy.....................................................................Passed
markdownlint.............................................................Passed
pydocstyle...............................................................Passed
flynt....................................................................Passed
codespell................................................................Passed

Edit: after rebasing, looks like the markdown errors were caused by another PR's change – I've fixed the linting errors in this PR.

@jtuglu1 jtuglu1 force-pushed the add-between-keyword branch from 5918924 to d0c11c8 Compare August 17, 2025 22:08
@jtuglu1 jtuglu1 force-pushed the add-between-keyword branch from d0c11c8 to 376ca2b Compare August 17, 2025 22:09
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.

Looks good, thanks for working on this @jtuglu1

Should we also add a small section on the expression page?

@jtuglu1
Copy link
Contributor Author

jtuglu1 commented Aug 18, 2025

Looks good, thanks for working on this @jtuglu1

Should we also add a small section on the expression page?

Sure. It doesn't appear that documentation for this belongs in expression-dsl.md since it is moreso an operator (similar to LIKE, IN and NOT IN). So, I've added it in row-filter-syntax.md. Does this sound ok?

@jtuglu1 jtuglu1 force-pushed the add-between-keyword branch from 3702b54 to 8223936 Compare August 18, 2025 15:30
@jtuglu1 jtuglu1 changed the title Add between keyword Add between operator Aug 18, 2025
@Fokko Fokko merged commit 8b43eb8 into apache:main Aug 18, 2025
10 checks passed
@Fokko
Copy link
Contributor

Fokko commented Aug 18, 2025

@jtuglu1 Yes, you're right. Thanks! And thanks @maytasm for the review 👍

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.

The between keyword seems to be missing from pyiceberg expression
4 participants