MDEV-38970: Streaming window functions step 1 - #5267
Conversation
and criteria validation function
There was a problem hiding this comment.
Code Review
This pull request introduces support for streaming window functions, allowing certain functions like row number, rank, and dense rank to be computed on the fly without materializing into a temporary table. The code review feedback highlights several critical issues, including potential null pointer dereferences (crashes) due to missing checks on partition_list and order_list, a logic bug in compare_order_lists when handling trailing constants, and an ignored return value in cursor setup. Additionally, the reviewer suggests caching the THD pointer to avoid expensive thread-local lookups in the performance-critical per-row execution path, and resolving an inconsistency in how default versus explicit frames are handled.
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.
just check ordering compatibility across window functions instead
| // i would skip this now | ||
| List<Cursor_manager> cursor_managers; | ||
| if (get_window_functions_required_cursors(thd, window_functions, | ||
| &cursor_managers)) |
There was a problem hiding this comment.
cursor_managers are not deallocated, other window functions do it via delete_elements()
There was a problem hiding this comment.
OK, now they are deallocated on the normal path (get_window_functions_required_cursors succeeds) but if get_window_functions_required_cursors returns an error, already pushed cursor_managers are not freed
|
in tests to avoid query text duplication. |
I thought about this actually (running queries side by side) but thought I would need an optimizer switch just for that, I will try |
| // this means the order by should be done in a temp table (it's real purpose | ||
| // is checking if order by references only the first non-const table in JOIN) | ||
|
|
||
| // i'm not very sure of this, simple_order might change later?? |
There was a problem hiding this comment.
Re-stating here what we discussed on Zulip (not to forget):
It looks like yes, it can change. See lines 3452-3467 - there is some sort of fall-back to materialization. Probably the change of simple_order shouldn't be a problem for previously chosen streaming path but the modified JOIN::order can be. Please look at how this can be worked around
There was a problem hiding this comment.
Working on this part now. From what I see, the cases where need_tmp can be true after we check in test_if_need_tmp_table are as you said around line 3467, where it checks if sorting has expressions that are expensive and falls back to sorting, and around ~3550, something about if a Group by exists and loose scan is used. Those cause problems (we sort more fields we don't care about) if we had already changed the order and we fall back to materialization. I'm looking into where else that can be a problem.
There was a problem hiding this comment.
I think the fix is just to defer this decision of replacing order as late as we're sure need_tmp won't change again. I thought about leaving this case and just streaming if the main query ORDER is the longer list, as the original MDEV-36593 suggests, where it says window functions should re-use scanning / order with the main query but I think we leave a lot of optimization on the table if we do that. The simplest case we can look at is if any window function has ordering but the main query does not. If we drop this then this case would materialize.
There was a problem hiding this comment.
After examining the logic, I think the current JOIN::order replacement is safe. It's worth commenting with something like: "Safe: streaming_wf_order_is_longer guarantees the main ORDER BY is a prefix of win_func_longest_order, so this widening always refines the requested order; and any later need_tmp=1 falls back to materialization, where the widened order is still valid."
And please amend the comment
/*
If window functions are present then we can't have simple_order set to
TRUE as the window function needs a temp table for computation.
ORDER BY is computed after the window function computation is done, so
the sort will be done on the temp table.
*/
several lines above as it doesn't represent cases of streaming window functions.
There was a problem hiding this comment.
Yes, I know that it would not hurt correctness because the window function order is a longer and compatible with the main query order if the swap happened. I was talking about it from an optimization point. If we already fall back to materialization, why sort by longer keys we don't need (the extra ones in the swapped order). I spent some time trying to see if such case exists, by looking at the two cases that fall back to materialization.
- line ~3462. It falls back when an 'expensive' function exists. I took some time to try and see if this is a case we care about. I arrived at a query like
SELECT pk, a, b, rank() OVER (ORDER BY a, slow(b), pk) AS r FROM t1 ORDER BY a, slow(b);Here I have a non-deterministic ,expensive, function, and the window function has the order prefix longer, so our loss here should be that we fallback to materialization, but our order key is (a, slow(b), pk) instead of (a, slow(b)). But this case, that we swap and then fallback, never happens, because the existence of such expensive function already makessimple_order=false, which forces materialization early in test_if_need_tmp_table(). - line ~3548. Here it materializes under
if (group_list)andif (ordered_index_usage != ordered_index_group_by), we don't care about this case because it's mutually exclusive to how we stream with group by. We materialize early if a group by exists and is not resolved with a loose index scan (which means an index covers the group by).
This took quite some time to verify but it's safe now (we don't even lose optimization) with the swap.
…when QUICK_SELECT can be used)
|
Streaming such queries can provide correct results only when the rows arriving at the streaming callback are already grouped which is only true for a single-table loose index scan ( This concern also applies not only to multi-table plans, but also to single-table plans that group via If we still want to cover |
While this does not hurt correctness (for innoDB at least), it runs a useless pass of process_row() over the last read row again.
Yes, you're right I focused on cases where group by should stream and missed this. I handled this and added some tests. I looked into what makes a group by use end_send other than using a loose index scan. The other case that happens is when GROUP BY is rewritten into an ORDER BY when a unique non-null index exists on the group key. But for now this is skipped explicitly whenever grouping exists, I am experimenting with it to make sure I don't miss something. |
This is the only case for which the rows arrive in group order and one row per group, hence no further grouping / aggregration is needed. The other case is when the GROUP BY is rewrittein into ORDER when the keys are covered by a unique non-null index, which is not covered in this commit.
This PR is the initial implementation of the streaming window functions path as part of my GSOC project.
It's still under development. I'll update this accordingly during development.
To first highlight what's not there yet and what's not working
Note: Many comments are there just for myself, and some namings are to be changed.
What's been added for now:
Window_funcs_sort_streamingobject, the naming comes from the fact that our criteria can be defined as having only oneWindow_funcs_sortobject only, hence thesortin the name. Contains:compute_window_func)process_rowmethod, intended to iterate the window functions and run for the current row.have_streaming_window_funcs(), a preparation time function to check the criteria for streaming, it checks:(rank() over(max(a)))have_streaming_window_funcs()in preparation time to know if streaming CAN be satisfied (anything else needing a temp table falls back to materialization)window_funcs_streaming_stepis added to theJOIN_TAB, analog towindow_funcs_step, responsible for setting up window functions and holding the state for the trackers and cursors across rows. (it points to aWindow_funcs_sort_streamingobject)end_compute_window_func()is attached to the last real table (last table is always real in this case), which callsprocess_rowon the current row in the join loop, and sends it to the client.As I said, this is initial, it has most of what we want to implement for streaming, but needs some cleaning up and some reviewing, as I reused much of the logic.