Uh oh!
There was an error while loading. Please reload this page.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous documentation here describes the relationship between
exprandintervalas being direct, but the idea shouldn't be thatexpr >= lower and expr < upper, It should be:udf(expr) <op> literallogically implies thatexprcan be directly compared withinterval.That is, if
udf(expr) <op> literalis true, then there is a transformation involvingexprandintervalthat is logically equivalent.Specifically:
floor(x) < 8, thenpreimageshould returninterval: [8, 9)such that the expression can be rewritten tox < 8.floor(x) < 8.3, thenpreimageshould returninterval: [8, 9)such that the expression can be rewritten tox < 9.Notice that both expressions yield the same interval because both
8and8.3are literals in that range, and have equivalent outputs (floor(8) == floor(8.3) == { for y in [8, 9): floor(y) }).Then,
rewrite_with_preimagemust accommodate the predicate operator (<in this case) to correctly transform the expression using the preimage interval and a boundary condition (is_boundary = floor(y) == y).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand this logic
For the example
floor(x) < 8.3I would expect there to be no preimage as defined here -- specifically there is no range of inputs for whichfloor(x)evaluates to8.3I agree that it is valid simplificaition to rewrite
floor(x) < 8.3tox < 9.0, but it seems different than "preimage" 🤔Maybe we just need to give it a different name (maybe that is what you have tried to do with
is_boundary)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW I checked with datafusion-cli and the
floor(x) < 8.3case is not optimized today (the preimage is not applied here)