maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr!

On Nov 21, Oleksandr Byelkin wrote:
> revision-id: 49b63dcc0482bcb0fc7f642b0c19c8e24740ab27 (mariadb-10.4.0-20-g49b63dcc048)
> parent(s): b5ac863f1494920b5e7035c9dfa0ebfdaa50a15d
> author: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> committer: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> timestamp: 2018-11-20 10:58:34 +0100
> message:
> 
> MDEV-15073: Generic UDAF parser code in server for windows functions
> 
> Added support for usual agreggate UDF (UDAF)
> Added remove() call support for more efficient window function processing
> Added example of aggregate UDF with efficient windows function support

Looks good, see a couple of minor comments below.

I wouldn't extend UDF API, really. It's ancient and it should be removed
and replaced with a new plugin type, not extended.

But if you still want to do it make sure it's documented
https://mariadb.com/kb/en/user-defined-functions/

> diff --git a/mysql-test/main/udf.test b/mysql-test/main/udf.test
> index c3a25c6bcce..e72f8b219af 100644
> --- a/mysql-test/main/udf.test
> +++ b/mysql-test/main/udf.test
> @@ -528,3 +528,62 @@ DROP FUNCTION METAPHON;
>  #INSERT INTO t1 (a) VALUES ('Hello');
>  #SELECT * FROM t1;
>  DROP TABLE t1;
> +
> +--echo
> +--echo MDEV-15073: Generic UDAF parser code in server for windows functions
> +--echo

use --echo #
so that your echo commands would stand out in the result file.
currently they look like an output from some query and it's confusing.

The test itself is good.

> diff --git a/sql/item_sum.cc b/sql/item_sum.cc
> index 0e52b2988a3..d19645b5020 100644
> --- a/sql/item_sum.cc
> +++ b/sql/item_sum.cc
> @@ -3235,6 +3235,25 @@ bool Item_udf_sum::add()
>    DBUG_RETURN(0);
>  }
>  
> +
> +bool Item_udf_sum::supports_removal()
> +{
> +  DBUG_ENTER("Item_udf_sum::supports_remove");
> +  DBUG_PRINT("info", ("support: %d", udf.supports_removal()));
> +  DBUG_RETURN(udf.supports_removal());
> +}
> +
> +
> +void Item_udf_sum::remove()
> +{
> +  my_bool tmp_null_value;
> +  DBUG_ENTER("Item_udf_sum::remove");
> +  udf.remove(&tmp_null_value);
> +  null_value= tmp_null_value;
> +  DBUG_VOID_RETURN;
> +}
> +
> +
>  void Item_udf_sum::cleanup()
>  {
>    /*
> diff --git a/sql/item_sum.h b/sql/item_sum.h
> index 1a21c257221..663dea1ed7b 100644
> --- a/sql/item_sum.h
> +++ b/sql/item_sum.h
> @@ -574,7 +574,7 @@ class Item_sum :public Item_func_or_sum
>    virtual bool add()= 0;
>    virtual bool setup(THD *thd) { return false; }
>  
> -  virtual bool supports_removal() const { return false; }
> +  virtual bool supports_removal() { return false; }

why?

>    virtual void remove() { DBUG_ASSERT(0); }

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx