Skip to content

MDEV-10838: Avoiding computation when window frame is always empty#5167

Open
EricHayter wants to merge 6 commits into
MariaDB:mainfrom
EricHayter:empty-window-opt
Open

MDEV-10838: Avoiding computation when window frame is always empty#5167
EricHayter wants to merge 6 commits into
MariaDB:mainfrom
EricHayter:empty-window-opt

Conversation

@EricHayter
Copy link
Copy Markdown

This PR adds a simple optimization for window functions of the form [ROWS | RANGE] BETWEEN N PRECEDING AND M PRECEDING (where N < M) or BETWEEN N FOLLOWING AND M FOLLOWING (where N > M). In both cases the frame is statically empty, so the temp file sort in window function execution is skipped entirely and NULLs are written directly to the output for each row.

Resolves MDEV-10838

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

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.

Comment thread sql/sql_window.cc
Comment on lines +150 to +151
DBUG_ASSERT(!lt->fix_fields_if_needed(thd, (Item**) &lt));
return lt->val_int() != 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

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**) &lt))
      return false;
    return lt->val_int() != 0;

@gkodinov gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Jun 3, 2026
@gkodinov gkodinov self-assigned this Jun 3, 2026
Copy link
Copy Markdown
Member

@gkodinov gkodinov left a comment

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.

Development

Successfully merging this pull request may close these issues.

2 participants