Skip to content
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

Fix typo in _ManifestEvalVisitor.visit_equal #6117

Merged
merged 2 commits into from
Nov 7, 2022
Merged

Conversation

ddrinka
Copy link
Contributor

@ddrinka ddrinka commented Nov 3, 2022

@Fokko here's another runtime bug I'm seeing while playing with this new Manifest evaluator (#5845)

@github-actions github-actions bot added the python label Nov 3, 2022
@@ -526,7 +526,7 @@ def visit_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
if lower > literal.value:
return ROWS_CANNOT_MATCH

upper = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
Copy link
Member

Choose a reason for hiding this comment

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

good finding 👍🏻.

nit: maybe good to have unit tests around these.

@Fokko
Copy link
Contributor

Fokko commented Nov 4, 2022

Thanks for spotting this one @ddrinka It looks like we also need a not-None check. I also noticed that this is being fixed in #6069. So I'll close this one for now. If you need this right away, feel free to open up a new PR (with the check included).

@Fokko Fokko closed this Nov 4, 2022
@rdblue rdblue reopened this Nov 7, 2022
@rdblue
Copy link
Contributor

rdblue commented Nov 7, 2022

I reopened this because I think it's a good idea to get it in independently. Thanks for finding this, @ddrinka!

@Fokko Fokko merged commit 157b707 into apache:master Nov 7, 2022
@Fokko
Copy link
Contributor

Fokko commented Nov 7, 2022

Fixed the linting issue, thanks @ddrinka!

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

Successfully merging this pull request may close these issues.

4 participants