MDEV-10838: Avoiding computation when window frame is always empty#5167
MDEV-10838: Avoiding computation when window frame is always empty#5167EricHayter wants to merge 6 commits into
Conversation
This is to avoid any unneccessary computation for window functions that are always empty
There was a problem hiding this comment.
Code Review
This pull request introduces an optimization for window functions by detecting when window frames are always empty, allowing the execution to bypass sorting and cursor management via a fast-path sequential scan. However, a critical bug was identified in Window_frame::is_frame_always_empty(), where fix_fields_if_needed is called inside a DBUG_ASSERT macro. In release builds, this assertion is compiled out, which would prevent the item from being fixed and likely lead to crashes or undefined behavior during execution.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| DBUG_ASSERT(!lt->fix_fields_if_needed(thd, (Item**) <)); | ||
| return lt->val_int() != 0; |
There was a problem hiding this comment.
Evaluating an expression with side effects (such as fix_fields_if_needed) inside a DBUG_ASSERT macro is a critical bug. In release/production builds where DBUG_OFF is defined, DBUG_ASSERT compiles to a no-op, meaning fix_fields_if_needed will never be called. This will cause the item to remain unfixed, leading to undefined behavior or crashes when val_int() is subsequently called. Instead, call fix_fields_if_needed outside of the assertion and handle any potential errors gracefully.
if (lt->fix_fields_if_needed(thd, (Item**) <))
return false;
return lt->val_int() != 0;
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
Please squash all of your commits into a single commit per logical change. This might be a single commit or two commits in your case: depends on how much you want to highlight the two logical optimizations you're doing (detect a single empty frame and detect a situation when all frames are empty).
Then please make sure the commit(s) have a good explanation in their commit message(s), compliant with the MariaDB server coding standards.
Please also consider adding a regression test for this.
One way of doing this that comes to mind is testing some sort of status counters (e.g. on I/O ops) post query that might get bumped or not.
Another way is to dip a DBUG_EXECUTE_IF() that will cause the execution to fail if it gets to the DBUG_EXECUTE_IF and it's on. Then you set the flag and do a test SQL hoping it won't fail (as in: go into the non-optimized code path).
If there's no possibility for a practical regression test, please explain why in the commit message.
This PR adds a simple optimization for window functions of the form
[ROWS | RANGE] BETWEEN N PRECEDING AND M PRECEDING(whereN<M) orBETWEEN N FOLLOWING AND M FOLLOWING(whereN>M). In both cases the frame is statically empty, so the temp file sort in window function execution is skipped entirely andNULLs are written directly to the output for each row.Resolves MDEV-10838