Hi Sergei, Thanks for the comments. See below see my response and I have posted the url to the updated patch in the ticket. On Tue 2023-06-20 17:04:50 +0200, Sergei Golubchik wrote:

Hi, Yuchen,

On Jun 20, Yuchen Pei wrote:

...

revision-id: f3aa4d0a215 (mariadb-10.4.30-4-gf3aa4d0a215) parent(s): f5dceafd0ba author: Yuchen Pei committer: Yuchen Pei timestamp: 2023-06-14 15:02:38 +1000 message:

MDEV-29447 MDEV-26285 MDEV-31338 Refactor spider_db_mbase_util::open_item_func

spider_db_mbase_util::open_item_func() is a monster function. It is difficult to maintain while it is expected that we need to modify it when a new SQL function or a new func_type is added.

We split the function into two distinct functions: one handles the case of str != NULL and the other handles the case of str == NULL.

This refactoring was done in a conservative way because we do not have comprehensive tests on the function.

It also fixes MDEV-29447 and MDEV-31338 where field items that are arguments of a func item may be used before created / initialised.

Note this commit is a port of 3836098c29ef1b7ff9d5fbde99b690eab73a0df1 (MDEV-26285) to current versions 10.3+.

As I wrote in the first review, please, don't mention transient commit hashes in a permanent commit comment. This commit will, supposedly, be pushed into the main branch. While 3836098c29ef1 will, on the other hand, disappear, as it's not in any of the main branches. You'll have a dangling pointer

Done.

...

Signed-off-by: Yuchen Pei <yuchen.pei@mariadb.com>

also, it looks strange to sign-off your own commits

I thought I needed this for gpg sign, but just learned they are two different things. So I have removed the sign-off. By the way why is it strange to sign-off own commits?

...

diff --git a/storage/spider/spd_db_mysql.cc b/storage/spider/spd_db_mysql.cc index a51c1497b02..1ed733105c6 100644 --- a/storage/spider/spd_db_mysql.cc +++ b/storage/spider/spd_db_mysql.cc @@ -5848,94 +5992,12 @@ int spider_db_mbase_util::open_item_func( { if (!strncasecmp("utc_timestamp", func_name, func_name_length)) { - if (str) str->length(str->length() - SPIDER_SQL_OPEN_PAREN_LEN); DBUG_RETURN(spider_db_open_item_string(item_func, NULL, spider, str, alias, alias_length, dbton_id, use_fields, fields)); } else if (!strncasecmp("timestampdiff", func_name, func_name_length)) { -#ifdef ITEM_FUNC_TIMESTAMPDIFF_ARE_PUBLIC

Why did you removed this and not the #else part?

This is part of MDEV-29269. It was a done task with a fixversion 11.0+, but there's no reason the same change shouldn't be in earlier versions. This is one of the redundant #ifdefs, where the flag is deterministically 0 or 1 (see MDEV-26178 for more context). In this case ITEM_FUNC_TIMESTAMPDIFF_ARE_PUBLIC is always undefined, so we remove the #if part and keep the #else part.

...

- Item_func_timestamp_diff *item_func_timestamp_diff = - (Item_func_timestamp_diff *) item_func; - if (str) - { - const char *interval_str; - uint interval_len; - switch (item_func_timestamp_diff->int_type) - { - case INTERVAL_YEAR: - interval_str = SPIDER_SQL_YEAR_STR; - interval_len = SPIDER_SQL_YEAR_LEN; - break; - case INTERVAL_QUARTER: - interval_str = SPIDER_SQL_QUARTER_STR; - interval_len = SPIDER_SQL_QUARTER_LEN; - break; - case INTERVAL_MONTH: - interval_str = SPIDER_SQL_MONTH_STR; - interval_len = SPIDER_SQL_MONTH_LEN; - break; - case INTERVAL_WEEK: - interval_str = SPIDER_SQL_WEEK_STR; - interval_len = SPIDER_SQL_WEEK_LEN; - break; - case INTERVAL_DAY: - interval_str = SPIDER_SQL_DAY_STR; - interval_len = SPIDER_SQL_DAY_LEN; - break; - case INTERVAL_HOUR: - interval_str = SPIDER_SQL_HOUR_STR; - interval_len = SPIDER_SQL_HOUR_LEN; - break; - case INTERVAL_MINUTE: - interval_str = SPIDER_SQL_MINUTE_STR; - interval_len = SPIDER_SQL_MINUTE_LEN; - break; - case INTERVAL_SECOND: - interval_str = SPIDER_SQL_SECOND_STR; - interval_len = SPIDER_SQL_SECOND_LEN; - break; - case INTERVAL_MICROSECOND: - interval_str = SPIDER_SQL_MICROSECOND_STR; - interval_len = SPIDER_SQL_MICROSECOND_LEN; - break; - default: - interval_str = ""; - interval_len = 0; - break; - } - str->length(str->length() - SPIDER_SQL_OPEN_PAREN_LEN); - if (str->reserve(func_name_length + SPIDER_SQL_OPEN_PAREN_LEN + - interval_len + SPIDER_SQL_COMMA_LEN)) - DBUG_RETURN(HA_ERR_OUT_OF_MEM); - str->q_append(func_name, func_name_length); - str->q_append(SPIDER_SQL_OPEN_PAREN_STR, SPIDER_SQL_OPEN_PAREN_LEN); - str->q_append(interval_str, interval_len); - str->q_append(SPIDER_SQL_COMMA_STR, SPIDER_SQL_COMMA_LEN); - } - if ((error_num = spider_db_print_item_type(item_list[0], NULL, spider, - str, alias, alias_length, dbton_id, use_fields, fields))) - DBUG_RETURN(error_num); - if (str) - { - if (str->reserve(SPIDER_SQL_COMMA_LEN)) - DBUG_RETURN(HA_ERR_OUT_OF_MEM); - str->q_append(SPIDER_SQL_COMMA_STR, SPIDER_SQL_COMMA_LEN); - } - if ((error_num = spider_db_print_item_type(item_list[1], NULL, spider, - str, alias, alias_length, dbton_id, use_fields, fields))) - DBUG_RETURN(error_num); - if (str) - { - if (str->reserve(SPIDER_SQL_CLOSE_PAREN_LEN)) - DBUG_RETURN(HA_ERR_OUT_OF_MEM); - str->q_append(SPIDER_SQL_CLOSE_PAREN_STR, - SPIDER_SQL_CLOSE_PAREN_LEN); - } - DBUG_RETURN(0); -#else DBUG_RETURN(ER_SPIDER_COND_SKIP_NUM); -#endif } } else if (func_name_length == 14) {

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org

Best, Yuchen