Fix the Window operator with overflowed empty k-rows frames #9476
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
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D56085211 |
✅ Deploy Preview for meta-velox canceled.
|
|
Hi @aditi-pandit, could you help review this PR? Thanks! |
aditi-pandit
approved these changes
Apr 16, 2024
kagamiori
force-pushed
the
export-D56085211
branch
from
c42c352
to
daea8a7
Compare
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Apr 16, 2024
…incubator#9476) Summary: The boundaries of Window frames are int32 integers. When the frame boundaries given in the query exceed int32 limit, the Window operator needs to adjust the frame bounds. However, the current code has a bug that when the frame end is below INT32_MIN, it adjust the frame end to 0. This is wrong because the original frame is empty, but after the adjustment, it always include row 0. This diff fixes this bug by setting the frame bound to -1 when the frame bound belows INT32_MIN. A subsequent call to computeValidFrames will check whether this frame is empty and mark it properly. This diff fixes facebookincubator#9375. Differential Revision: D56085211
|
This pull request was exported from Phabricator. Differential Revision: D56085211 |
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Apr 16, 2024
…incubator#9476) Summary: The boundaries of Window frames are int32 integers. When the frame boundaries given in the query exceed int32 limit, the Window operator needs to adjust the frame bounds. However, the current code has a bug that when the frame end is below INT32_MIN, it adjust the frame end to 0. This is wrong because the original frame is empty, but after the adjustment, it always include row 0. This diff fixes this bug by setting the frame bound to -1 when the frame bound belows INT32_MIN. A subsequent call to computeValidFrames will check whether this frame is empty and mark it properly. This diff fixes facebookincubator#9375. Differential Revision: D56085211
kagamiori
force-pushed
the
export-D56085211
branch
from
daea8a7
to
da58d6c
Compare
|
This pull request was exported from Phabricator. Differential Revision: D56085211 |
Yuhta
approved these changes
Apr 16, 2024
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Apr 16, 2024
…incubator#9476) Summary: The boundaries of Window frames are int32 integers. When the frame boundaries given in the query exceed int32 limit, the Window operator needs to adjust the frame bounds. However, the current code has a bug that when the frame end is below INT32_MIN, it adjust the frame end to 0. This is wrong because the original frame is empty, but after the adjustment, it always include row 0. This diff fixes this bug by setting the frame bound to -1 when the frame bound belows INT32_MIN. A subsequent call to computeValidFrames will check whether this frame is empty and mark it properly. This diff fixes facebookincubator#9375. Reviewed By: Yuhta Differential Revision: D56085211
kagamiori
force-pushed
the
export-D56085211
branch
from
da58d6c
to
4803b58
Compare
|
This pull request was exported from Phabricator. Differential Revision: D56085211 |
…incubator#9476) Summary: The boundaries of Window frames are int32 integers. When the frame boundaries given in the query exceed int32 limit, the Window operator needs to adjust the frame bounds. However, the current code has a bug that when the frame end is below INT32_MIN, it adjust the frame end to 0. This is wrong because the original frame is empty, but after the adjustment, it always include row 0. This diff fixes this bug by setting the frame bound to -1 when the frame bound belows INT32_MIN. A subsequent call to computeValidFrames will check whether this frame is empty and mark it properly. This diff fixes facebookincubator#9375. Reviewed By: Yuhta Differential Revision: D56085211
kagamiori
force-pushed
the
export-D56085211
branch
from
4803b58
to
f009024
Compare
|
This pull request was exported from Phabricator. Differential Revision: D56085211 |
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Apr 16, 2024
…incubator#9476) Summary: The boundaries of Window frames are int32 integers. When the frame boundaries given in the query exceed int32 limit, the Window operator needs to adjust the frame bounds. However, the current code has a bug that when the frame end is below INT32_MIN, it adjust the frame end to 0. This is wrong because the original frame is empty, but after the adjustment, it always include row 0. This diff fixes this bug by setting the frame bound to -1 when the frame bound belows INT32_MIN. A subsequent call to computeValidFrames will check whether this frame is empty and mark it properly. This diff fixes facebookincubator#9375. Reviewed By: Yuhta Differential Revision: D56085211
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Apr 16, 2024
…incubator#9476) Summary: The boundaries of Window frames are int32 integers. When the frame boundaries given in the query exceed int32 limit, the Window operator needs to adjust the frame bounds. However, the current code has a bug that when the frame end is below INT32_MIN, it adjust the frame end to 0. This is wrong because the original frame is empty, but after the adjustment, it always include row 0. This diff fixes this bug by setting the frame bound to -1 when the frame bound belows INT32_MIN. A subsequent call to computeValidFrames will check whether this frame is empty and mark it properly. This diff fixes facebookincubator#9375. Reviewed By: Yuhta Differential Revision: D56085211
|
This pull request has been merged in 79f3add. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
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.
Summary:
The boundaries of Window frames are int32 integers. When the frame
boundaries given in the query exceed int32 limit, the Window
operator needs to adjust the frame bounds. However, the current
code has a bug that when the frame end is below INT32_MIN, it
adjust the frame end to 0. This is wrong because the original frame is
empty, but after the adjustment, it always include row 0. This diff fixes
this bug by setting the frame bound to -1 when the frame bound
belows INT32_MIN. A subsequent call to computeValidFrames will
check whether this frame is empty and mark it properly.
This diff fixes #9375.
Differential Revision: D56085211