Skip to content

MDEV-38970: Streaming window functions step 1 - #5267

Draft
OmarGamal10 wants to merge 16 commits into
MariaDB:mainfrom
OmarGamal10:mdev-38970
Draft

MDEV-38970: Streaming window functions step 1#5267
OmarGamal10 wants to merge 16 commits into
MariaDB:mainfrom
OmarGamal10:mdev-38970

Conversation

@OmarGamal10

@OmarGamal10 OmarGamal10 commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

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

  • Tests are still being added and refined.
  • Working on streaming aggregate functions (sum/count/mean/avg/min/max) when the frame is explicitly the current row only.

Note: Many comments are there just for myself, and some namings are to be changed.

What's been added for now:

  • A Window_funcs_sort_streaming object, the naming comes from the fact that our criteria can be defined as having only one Window_funcs_sort object only, hence the sort in the name. Contains:
    • A setup function for setting up trackers, cursor managers and setting phase to computation (like how it's done in compute_window_func)
    • process_row method, 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:
    • All window functions have a compatible ordering criteria
    • No aggregation functions exist within any window function (rank() over(max(a)))
    • Frame is default (unbounded preceding) or current row only.
    • Whether the window function sort order is compatible with the main query ORDER BY, and which is longer.
  • We run have_streaming_window_funcs() in preparation time to know if streaming CAN be satisfied (anything else needing a temp table falls back to materialization)
  • A window_funcs_streaming_step is added to the JOIN_TAB, analog to window_funcs_step, responsible for setting up window functions and holding the state for the trackers and cursors across rows. (it points to a Window_funcs_sort_streaming object)
  • end_compute_window_func() is attached to the last real table (last table is always real in this case), which calls process_row on 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.

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

Copy link
Copy Markdown
Contributor

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 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.

Comment thread sql/sql_window.cc
Comment thread sql/sql_window.cc
Comment thread sql/sql_window.cc Outdated
Comment thread sql/sql_window.cc
Comment thread sql/sql_window.cc Outdated
Comment thread sql/sql_window.cc Outdated
Comment thread sql/sql_window.cc
Comment thread sql/sql_window.cc Outdated
Comment thread sql/sql_window.h
@Olernov Olernov added the GSoC label Jun 23, 2026
@Olernov
Olernov self-requested a review June 23, 2026 09:38
@gkodinov gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Jun 23, 2026
Comment thread sql/sql_select.cc Outdated
Comment thread sql/sql_window.cc
Comment thread sql/sql_window.cc
// i would skip this now
List<Cursor_manager> cursor_managers;
if (get_window_functions_required_cursors(thd, window_functions,
&cursor_managers))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cursor_managers are not deallocated, other window functions do it via delete_elements()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@mariadb-OlegSmirnov

mariadb-OlegSmirnov commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

win_streaming.test does not prove correctness of results. Would be nice to see the same query executed in both variants: materialized vs streaming so results are comparable (maybe we can dismiss the streaming by adding SQL_BUFFER_RESULT after SELECT?).
As I mentioned earlier, do we really need EXPLAIN FORMAT=JSON everywhere or EXPLAIN EXTENDED is enough?
I also suggest using the pattern

let $q = SELECT ...;
eval $q;
eval explain format=json $q;

in tests to avoid query text duplication.

Comment thread sql/sql_window.cc Outdated
Comment thread sql/sql_window.cc Outdated
Comment thread sql/sql_window.cc
Comment thread sql/sql_window.cc Outdated
Comment thread sql/sql_window.cc
Comment thread sql/sql_window.cc
@OmarGamal10

Copy link
Copy Markdown
Contributor Author

win_streaming.test does not prove correctness of results. Would be nice to see the same query executed in both variants: materialized vs streaming so results are comparable (maybe we can dismiss the streaming by adding SQL_BUFFER_RESULT after SELECT?). As I mentioned earlier, do we really need EXPLAIN FORMAT=JSON everywhere or EXPLAIN EXTENDED is enough? I also suggest using the pattern

let $q = SELECT ...;
eval $q;
eval explain format=json $q;

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 SQL_BUFFER_RESULT

Comment thread sql/sql_select.cc Outdated
// 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??

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@OmarGamal10 OmarGamal10 Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@OmarGamal10 OmarGamal10 Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 makes simple_order=false, which forces materialization early in test_if_need_tmp_table().
  • line ~3548. Here it materializes under if (group_list) and if (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.

Comment thread sql/sql_window.cc Outdated
Comment thread sql/sql_window.cc Outdated
Comment thread sql/sql_window.cc Outdated
@Olernov

Olernov commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

GROUP_BY relaxation breaks multi-table joins (wrong results). Repro:

CREATE TABLE tg (a INT, b INT, KEY(a,b));
INSERT INTO tg VALUES (1,1),(1,2),(2,1),(2,2),(2,3),(3,1);
CREATE TABLE t2 (a INT, x INT, KEY(a));
INSERT INTO t2 VALUES (1,10),(2,20),(2,21),(3,30);

SELECT tg.a, rank() OVER (ORDER BY tg.a) FROM tg JOIN t2 ON tg.a=t2.a GROUP BY tg.a;
--   streamed:      9 rows, a = NULL everywhere, rnk = 1,1,3,3,3,3,3,3,9
SELECT SQL_BUFFER_RESULT tg.a, rank() OVER (ORDER BY tg.a) FROM tg JOIN t2 ON tg.a=t2.a GROUP BY tg.a;
--   materialized:  3 rows, 1 -> 1, 2 -> 2, 3 -> 3   (correct)

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 (using_index_for_group_by). For a multi-table join, grouping is performed by end_send_group, which the streaming attach overrides when it sets last_real_tab->next_select = end_compute_win_func. As a result, the grouping is silently bypassed.

This concern also applies not only to multi-table plans, but also to single-table plans that group via end_send_group. The last table emits ungrouped rows and the grouping executor collapses, overriding its next_select throws the grouping away.

If we still want to cover GROUP BY cases, the eligibility of streaming must be tightened, for example: confirm the plan is using_index_for_group_by and there's a single non-const table, rather than inferring it from simple_group/!need_tmp.

Comment thread sql/sql_select.cc Outdated
While this does not hurt correctness (for innoDB at least), it runs a
useless pass of process_row() over the last read row again.
@OmarGamal10

Copy link
Copy Markdown
Contributor Author

GROUP_BY relaxation breaks multi-table joins (wrong results). Repro:

CREATE TABLE tg (a INT, b INT, KEY(a,b));
INSERT INTO tg VALUES (1,1),(1,2),(2,1),(2,2),(2,3),(3,1);
CREATE TABLE t2 (a INT, x INT, KEY(a));
INSERT INTO t2 VALUES (1,10),(2,20),(2,21),(3,30);

SELECT tg.a, rank() OVER (ORDER BY tg.a) FROM tg JOIN t2 ON tg.a=t2.a GROUP BY tg.a;
--   streamed:      9 rows, a = NULL everywhere, rnk = 1,1,3,3,3,3,3,3,9
SELECT SQL_BUFFER_RESULT tg.a, rank() OVER (ORDER BY tg.a) FROM tg JOIN t2 ON tg.a=t2.a GROUP BY tg.a;
--   materialized:  3 rows, 1 -> 1, 2 -> 2, 3 -> 3   (correct)

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 (using_index_for_group_by). For a multi-table join, grouping is performed by end_send_group, which the streaming attach overrides when it sets last_real_tab->next_select = end_compute_win_func. As a result, the grouping is silently bypassed.

This concern also applies not only to multi-table plans, but also to single-table plans that group via end_send_group. The last table emits ungrouped rows and the grouping executor collapses, overriding its next_select throws the grouping away.

If we still want to cover GROUP BY cases, the eligibility of streaming must be tightened, for example: confirm the plan is using_index_for_group_by and there's a single non-const table, rather than inferring it from simple_group/!need_tmp.

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.

  /*
     Check if we can optimize away GROUP BY/DISTINCT.
     We can do that if there are no aggregate functions, the
     fields in DISTINCT clause (if present) and/or columns in GROUP BY
     (if present) contain direct references to all key parts of
     an unique index (in whatever order) and if the key parts of the
     unique index cannot contain NULLs.
     Note that the unique keys for DISTINCT and GROUP BY should not
     be the same (as long as they are unique).

     The FROM clause must contain a single non-constant table.
  */
  if (table_count - const_tables == 1 && (group || select_distinct) &&
      !tmp_table_param.sum_func_count &&
      (!join_tab[const_tables].select ||
       !join_tab[const_tables].select->quick ||
       join_tab[const_tables].select->quick->get_type() != 
       QUICK_SELECT_I::QS_TYPE_GROUP_MIN_MAX) &&
      !select_lex->have_window_funcs())

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.
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. GSoC

Development

Successfully merging this pull request may close these issues.

4 participants