maria-developers team mailing list archive

[Sergey Petrunya <psergey@xxxxxxxxxxxx>]

Hello Igor,

Plese find the comments below.

On Sat, Oct 30, 2010 at 12:56:10PM -0700, Igor Babaev wrote:
> At file:///home/igor/maria/maria-5.3-mwl128-bug668290/
> 
> ------------------------------------------------------------
> revno: 2840
> revision-id: igor@xxxxxxxxxxxx-20101030195609-h53be2cdzjlz38bz
> parent: igor@xxxxxxxxxxxx-20101030130745-563ypxtkcwv4vbfg
> committer: Igor Babaev <igor@xxxxxxxxxxxx>
> branch nick: maria-5.3-mwl128-bug668290
> timestamp: Sat 2010-10-30 12:56:09 -0700
> message:
>   Fixed LP bug #668290.
>   Prohibited to use hash join algorithm BNLH if join attributes
>   need non-binary collations. It has to be done because BNLH does
>   not support join for such attributes yet.
>   Later this limitations will be lifted.
>   
>   Changed default collations for the schemes of some test cases
>   to preserve the old execution plans.
...
> === modified file 'sql/field.h'
> --- a/sql/field.h	2010-09-23 22:00:32 +0000
> +++ b/sql/field.h	2010-10-30 19:56:09 +0000
> @@ -585,6 +585,12 @@
>    }
>    /* Hash value */
>    virtual void hash(ulong *nr, ulong *nr2);
> +
> +  virtual bool hash_join_is_possible(bool globally_allowed)
> +  {
> +    return globally_allowed;
> +  }
Is there any partiular reason why have this globally_allowed parameter? 

In my understanding, a field either allows hash or not, and that is 
regardless of whether it was "globally allowed", so the check of whether
hash join is globally allowed should be done outside of the function.

Also, it would be nice to have here a comment specifying the meaning of this
function. (I know all this is supposed to be temporary, but expirience shows
that often supposedly temporary things stay for a long time, that's why it's
nice to put a comment).
> +
>    friend bool reopen_table(THD *,struct st_table *,bool);
>    friend int cre_myisam(char * name, register TABLE *form, uint options,
>  			ulonglong auto_increment_value);
> @@ -760,6 +766,12 @@
>    my_decimal *val_decimal(my_decimal *);
>    virtual bool str_needs_quotes() { return TRUE; }
>    uint is_equal(Create_field *new_field);
> +
> +  bool hash_join_is_possible(bool globally_allowed)
> +  {
> +    /* TODO: support hash joins for non-binary collations */
> +    return globally_allowed && (flags & BINARY_FLAG);
> +  }
>  };
>  
>  
> @@ -1904,6 +1916,7 @@
>    uint size_of() const { return sizeof(*this); }
>    int  reset(void) { return !maybe_null() || Field_blob::reset(); }
>    geometry_type get_geometry_type() { return geom_type; };
> +  bool hash_join_is_possible(bool globally_allowed) { return FALSE; }
>  };
>  #endif /*HAVE_SPATIAL*/
>  
> 
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2010-10-30 13:07:45 +0000
> +++ b/sql/sql_select.cc	2010-10-30 19:56:09 +0000
> @@ -3547,7 +3547,7 @@
>          /* 
>            Additional optimization: if we're processing
>            "t.key BETWEEN c1 AND c1" then proceed as if we were processing
> -          "t.key = c1".
> +          "t.key = c1".sis
>            TODO: This is a very limited fix. A more generic fix is possible. 
Is the above a meaningful change?

>            There are 2 options:
>            A) Make equality propagation code be able to handle BETWEEN
> @@ -5946,6 +5946,44 @@
>  }
>  
>  
> +/**
> +  @brief
> +  Check whether hash join algorithm can be used to join this table
> +
> +  @param is_allowed      usage of hash join is allowed   
> +
> +  @details
> +  This function finds out whether the ref items that have been chosen
> +  by the planner to access this table can be used for hash join algorithms.
> +  The answer depends on a certain property of the the fields of the
> +  joined tables on which the hash join key is built. If hash join is
> +  allowed for all these fields the answer is positive unless hash join
> +  algorithms are not allowed for the query at all.
> +  
> +  @note
> +  The function is supposed to be called now only after the function
> +  get_best_combination has been called.
> +
> +  @retval TRUE    it's possible to use hash join to join this table
> +  @retval FALSE   otherwise
> +*/
> +
> +bool JOIN_TAB::hash_join_is_possible(bool is_allowed)
> +{
> +  if (!is_allowed)
> +    return FALSE;
Same as with globally_allowed. I think the parameter is redundant and rather
confusing.

> +  if (type != JT_REF && type != JT_EQ_REF)
> +    return FALSE;
> +  KEY *keyinfo= &table->key_info[ref.key];
> +  for (uint i= 0; i < ref.key_parts; i++)
> +  {
> +    if (!(keyinfo->key_part[i].field->hash_join_is_possible(TRUE)))
> +      return FALSE;
> +  }
> +  return TRUE;
> +}
> +
> +
>  static uint
>  cache_record_length(JOIN *join,uint idx)
>  {
> @@ -7649,8 +7687,10 @@
>                                                    &bufsz, &flags, &cost);
>  
>      if ((cache_level <=4 && !no_hashed_cache) || no_bka_cache ||
> -         (flags & HA_MRR_NO_ASSOCIATION) && cache_level <=6)
> +	((flags & HA_MRR_NO_ASSOCIATION) && cache_level <=6))
>      {
> +      if (!tab->hash_join_is_possible(TRUE))
> +        goto no_join_cache;
>        if (cache_level == 3)
>          prev_cache= 0;
>        if ((tab->cache= new JOIN_CACHE_BNLH(join, tab, prev_cache)) &&
> 
> === modified file 'sql/sql_select.h'
> --- a/sql/sql_select.h	2010-10-30 13:07:45 +0000
> +++ b/sql/sql_select.h	2010-10-30 19:56:09 +0000
> @@ -391,6 +391,7 @@
>      return max_used_fieldlength;
>    }
>    double get_partial_join_cardinality() { return partial_join_cardinality; }
> +  bool hash_join_is_possible(bool is_allowed);
>  } JOIN_TAB;
>  
>  
> 
 
B8R
 Sergey
-- 
Sergey Petrunia, Software Developer
Monty Program AB, http://askmonty.org
Blog: http://s.petrunia.net/blog