maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Oleg,

Please find below coding style/etc input re the patch for MDEV-25080.
(See the MDEV for other comments, more are coming).

> diff --git a/sql/select_handler.cc b/sql/select_handler.cc
> index 795ed8eb641..99bd2c0cf7f 100644
> --- a/sql/select_handler.cc
> +++ b/sql/select_handler.cc
> @@ -114,10 +125,7 @@ bool select_handler::send_data()
>  bool select_handler::send_eof()
>  {
>    DBUG_ENTER("select_handler::send_eof");
> -
> -  if (select->join->result->send_eof())
> -    DBUG_RETURN(true);
> -  DBUG_RETURN(false);
> +  DBUG_RETURN(result->send_eof());
>  }

What is this change for? If it's just the code cleanup and we HAVE to do it (I 
don't see an urgent reason), please do it in a separate commit.

Reasons against putting this change into this commit: 
When one sees this change as part of a meaningful patch, they start to 
examine the change to see what difference it makes. After one minute,
one realizes the old and new code are the same. The time and attention
have already been wasted, though.

> diff --git a/sql/sql_explain.cc b/sql/sql_explain.cc
> index 7c3918bfd20..6726e1b28f1 100644
> --- a/sql/sql_explain.cc
> +++ b/sql/sql_explain.cc
> @@ -555,6 +575,12 @@ int Explain_union::print_explain(Explain_query *query,
>    }
>  
>    if (!using_tmp)
> +    /*
> +      The union operation may not employ a temporary table, for example,
> +      for UNION ALL, in that case the results of the query are sent directly
> +      to the output. So there is no actual UNION operation and we don't need
> +      to print the line in the EXPLAIN output.
> +    */
>      return 0;

Please add { } braces as the if-part takes more than one line now.

> diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc
> index 9382625a747..437acbb749c 100644
> --- a/sql/sql_lex.cc
> +++ b/sql/sql_lex.cc
> @@ -5549,9 +5552,13 @@ void st_select_lex::set_explain_type(bool on_the_fly)
>            type= "MATERIALIZED UNION";
>          else
>          {
> -          type= is_uncacheable ? "UNCACHEABLE UNION": "UNION";
> +          type= is_uncacheable ? "UNCACHEABLE UNION"
> +                                : "UNION";
>
Again: please don't just re-format the code

> @@ -5561,15 +5568,14 @@ void st_select_lex::set_explain_type(bool on_the_fly)
>            {
>              bool uses_cte= false;
>              for (JOIN_TAB *tab= first_linear_tab(join, WITHOUT_BUSH_ROOTS,
> -                                                       WITH_CONST_TABLES);
> -                 tab;
> -                 tab= next_linear_tab(join, tab, WITHOUT_BUSH_ROOTS))
> +                                                  WITH_CONST_TABLES);
> +                  tab; tab= next_linear_tab(join, tab, WITHOUT_BUSH_ROOTS))
>              {
and here.


> diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> index 45fed46e375..2310583074b 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -1843,6 +1843,8 @@ int JOIN::optimize()
>    join_optimization_state init_state= optimization_state;
>    if (select_lex->pushdown_select)
>    {
> +    if (optimization_state == JOIN::OPTIMIZATION_DONE)
> +      return 0;

Please add and assert that after this we have
optimization_state==JOIN::NOT_OPTIMIZED.

>      // Do same as JOIN::optimize_inner does:
>      fields= &select_lex->item_list;
>  
> @@ -4885,7 +4887,6 @@ void JOIN::cleanup_item_list(List<Item> &items) const
>    DBUG_VOID_RETURN;
>  }
>  
> -
Please put this back, the coding style says to use two empty lines between the
functions.

>  /**
>    @brief
>      Look for provision of the select_handler interface by a foreign engine

..

> -select_handler *find_select_handler(THD *thd,
> -                                    SELECT_LEX* select_lex)
> +select_handler *find_select_handler(THD *thd, SELECT_LEX *select_lex)

^ Don't reformat.

>  {
> -  if (select_lex->next_select())
> -    return 0;
>    if (select_lex->master_unit()->outer_select())
>      return 0;
>  
> @@ -4921,15 +4922,14 @@ select_handler *find_select_handler(THD *thd,
>    {
>      tbl= select_lex->join->tables_list;
>    }
> -  else if (thd->lex->query_tables &&
> -           thd->lex->query_tables->next_global)
> +  else if (thd->lex->query_tables && thd->lex->query_tables->next_global)
>    {
>      tbl= thd->lex->query_tables->next_global;
>    }
>    else
>      return 0;
>  
> -  for (;tbl; tbl= tbl->next_global)
> +  for (; tbl; tbl= tbl->next_global)
>    {
>      if (!tbl->table)
>        continue;
> @@ -4937,12 +4937,14 @@ select_handler *find_select_handler(THD *thd,
>      if (!ht->create_select)
>        continue;
>      select_handler *sh= ht->create_select(thd, select_lex);
> -    return sh;
> +    if (sh)
> +      return sh;
>    }
>    return 0;
>  }
Don't reformat. meaningful changes are ok.
>  
>
> +

Remove the 3rd empty line.

>  /**
>    An entry point to single-unit select (a select without UNION).
>  
...

diff --git a/sql/sql_union.cc b/sql/sql_union.cc
index e8bdbcba2f2..4cb9f3e025a 100644
--- a/sql/sql_union.cc
+++ b/sql/sql_union.cc
@@ -1294,6 +1294,52 @@ bool init_item_int(THD* thd, Item_int* &item)
   return true;
 }
 
> +select_handler *find_unit_handler(THD *thd,
> +                                          SELECT_LEX_UNIT *unit)

^^^ coding style.

...
> @@ -2053,6 +2106,7 @@ void st_select_lex_unit::optimize_bag_operation(bool is_outer_distinct)
>    bag_set_op_optimized= true;
>  }
>  
> +select_handler *find_select_handler(THD *thd, SELECT_LEX *select_lex);

Please don't have function declaration in the middle of a file...

> @@ -2272,14 +2350,14 @@ bool st_select_lex_unit::exec()
>           if (union_result->flush())
>           {
>             thd->lex->current_select= lex_select_save;
> -	    DBUG_RETURN(1);
> +        return true;

Please fix identation.

> diff --git a/storage/federatedx/federatedx_pushdown.cc b/storage/federatedx/federatedx_pushdown.cc
> index 664f0570238..492055773cb 100644
> --- a/storage/federatedx/federatedx_pushdown.cc
> +++ b/storage/federatedx/federatedx_pushdown.cc
> @@ -163,13 +163,13 @@ void ha_federatedx_derived_handler::print_error(int, unsigned long)
>  }
>  
>  
> -static select_handler*
> -create_federatedx_select_handler(THD* thd, SELECT_LEX *sel)
> +template<typename T>
> +static select_handler *create_federatedx_handler(THD *thd, T *sel_lex)
>  {
>  if (!use_pushdown)
>    return 0;
>

Do we need to generate that much code? What if there was a function like 

  bool check_pushdown_allowed(select_lex_node *node)

which could process both SELECT_LEX and SELECT_LEX_UNIT objects. Then,
we would need two one-liner functions:

create_select_handler(type S) {
  if (check_pushdown_allowed(S))
    return new ha_federatedx_select_handler(S)
}


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