maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Varun,

- Please add a testcase to the testsuite
- Please address a few cosmetic comments below
- Ok to push after addressed.

> commit 5e448b77a3812e65623c0a1214049322ace3aacf
> Author: Varun Gupta <varun.gupta@xxxxxxxxxxx>
> Date:   Tue May 5 20:44:43 2020 +0530
> 
>     MDEV-22461: JOIN::make_aggr_tables_info(): Assertion `select_options & (1ULL << 17)' failed.
>     
>     A temporary table is needed for window function computation but if only a NAMED WINDOW SPEC
>     is used and there is no window function, then there is no need to create a temporary
>     table as there is no stage to compute WINDOW FUNCTION
> 
> diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> index c601946cfa0..0a1d0c2dbcc 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -2014,11 +2014,16 @@ JOIN::optimize_inner()
>    }
>  
>    need_tmp= test_if_need_tmp_table();
> -  //TODO this could probably go in test_if_need_tmp_table.
> -  if (this->select_lex->window_specs.elements > 0) {
> -    need_tmp= TRUE;
> +
> +  /*
> +    If window functions are present then we can't have simple_order set to
> +    TRUE as the window function needs a temp table for compuatation.
typo: compuatation.

> +    ORDER BY is computed after the window function computation is done, so
> +    the sort would be done on the temp table.
> +  */
> +  if (this->select_lex->have_window_funcs())

Is there a need to have "this->select_lex"?  I think not, please remove, as 
it only confuses the reader.

>      simple_order= FALSE;
> -  }
> +
>  
>    /*
>      If the hint FORCE INDEX FOR ORDER BY/GROUP BY is used for the table
> diff --git a/sql/sql_select.h b/sql/sql_select.h
> index 0e011c9267a..7a892c1af89 100644
> --- a/sql/sql_select.h
> +++ b/sql/sql_select.h
> @@ -1645,7 +1645,8 @@ class JOIN :public Sql_alloc
>  	    ((select_distinct || !simple_order || !simple_group) ||
>  	     (group_list && order) ||
>               MY_TEST(select_options & OPTION_BUFFER_RESULT))) ||
> -            (rollup.state != ROLLUP::STATE_NONE && select_distinct));
> +            (rollup.state != ROLLUP::STATE_NONE && select_distinct) ||
> +            select_lex->have_window_funcs());

Please amend the function comment above this code to cover the new condition as
well.

>    }
>    bool choose_subquery_plan(table_map join_tables);
>    void get_partial_cost_and_fanout(int end_tab_idx,
> 

BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog