[native] Fix crash in window queries without ORDER BY clause#18895
Conversation
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
@wuxueyang96 Thank you for fixing this bug.
presto-native-execution/presto_cpp/main/types/PrestoToVeloxQueryPlan.cpp
Outdated
Show resolved
Hide resolved
|
@mbasmanova PTAL |
|
@wuxueyang96 What is PTAL? |
presto-native-execution/presto_cpp/main/types/tests/PlanConverterTest.cpp
Outdated
Show resolved
Hide resolved
PTAL is short for |
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
@wuxueyang96 Thank you for adding an e2e test. Let's remove the original unit test that uses hard-coded JSON file, squash commits and write a nice commit message. Check out How to Write Better Git Commit Messages – A Step-By-Step Guide and How to Write a Git Commit Message to learn more about how to write good commit messages.
...o-native-execution/src/test/java/com/facebook/presto/nativeworker/TestHiveWindowQueries.java
Outdated
Show resolved
Hide resolved
...o-native-execution/src/test/java/com/facebook/presto/nativeworker/TestHiveWindowQueries.java
Outdated
Show resolved
Hide resolved
presto-native-execution/presto_cpp/main/types/tests/PlanConverterTest.cpp
Outdated
Show resolved
Hide resolved
wuxueyang96
force-pushed
the
fix_coredump_bug_for_window_node
branch
2 times, most recently
from
e77760a to
28f65ec
Compare
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
@wuxueyang96 Thank you for iterating on this PR. The code looks good. The commit message needs some improvement. How about,
[native] Fix crash in window queries without ORDER BY clause
Presto-to-Velox plan conversion logic didn't handle the case of
window clause without ORDER BY causing a crash.
mbasmanova
changed the title
Presto-to-Velox plan conversion logic didn't handle the case of window clause without ORDER BY causing a crash.
wuxueyang96
force-pushed
the
fix_coredump_bug_for_window_node
branch
from
28f65ec to
45045e0
Compare
This commit message looks better. Thanks. |
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
@wuxueyang96 Thank you for fixing this crash.
|
@wuxueyang96 Ping me once CI is green so I can merge the PR. |
CI is green now. Thank you! |
|
@wuxueyang96 Thank you for the contribution. |
3 participants
Presto-to-Velox plan conversion logic didn't handle the case of
window clause without ORDER BY causing a crash.