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
Signed-off-by: Yuchen Pei <yuchen.pei@mariadb.com>
also, it looks strange to sign-off your 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?
- 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