WIP Bb main mdev 15621#5111
Conversation
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Range Interval Partitioning for RANGE COLUMNS tables, allowing for the automatic creation of partitions based on a specified temporal interval during DML operations. The implementation involves new logic for calculating and adding partitions during the table opening process, along with parser updates to support the INTERVAL syntax. Critical feedback was provided regarding a potential infinite loop if the interval is zero, a null pointer dereference risk when encountering MAXVALUE partitions, and the omission of error checking for partition count calculations. Furthermore, the reviewer suggested using session timezones for temporal calculations and identified risky bitmap manipulation that should be replaced with a safer approach.
mariadb-YuchenPei
force-pushed
the
bb-main-mdev-15621
branch
from
249ce05 to
d162336
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces range interval partitioning for RANGE COLUMNS tables, enabling the automatic creation of new partitions based on a specified time interval during DML operations such as INSERT, UPDATE, and LOAD DATA. The implementation includes parser support for the INTERVAL syntax, logic to calculate and add new partitions dynamically, and comprehensive test cases. Review feedback identifies a critical issue where newly added partitions are not correctly marked with the PART_TO_BE_ADDED state, which would cause them to be skipped during constant initialization. Additionally, several improvements were suggested regarding type safety—specifically avoiding unsafe pointer casts for warning variables—and ensuring the consistent use of boolean literals instead of integers for boolean types.
change p_column_list_val::fixed to a bool remove redundant end label in partition_info::fix_column_value_functions
mariadb-YuchenPei
force-pushed
the
bb-main-mdev-15621
branch
from
d162336 to
0afea08
Compare
Allow auto partitioning by interval in PARTITION BY RANGE COLUMNS PARTITION BY RANGE COLUMNS (col_name) INTERVAL interval [AUTO] ( PARTITION partition_name VALUES LESS THAN (value) [, PARTITION partition_name VALUES LESS THAN (value) ... ] ) where - col_name is the name of one column of type DATE or DATETIME - at least one partition is supplied - INTERVAL interval is a time interval When performing DML on such a table, it will first add partitions by the specified interval until the partition covers the current time. Partition addition will not cause an implicit commit like DDL normally does. The partitions are named pN. Otherwise the table behaves exactly the same as a normal RANGE COLUMNS partitioned table.
mariadb-YuchenPei
force-pushed
the
bb-main-mdev-15621
branch
from
0afea08 to
0ee340c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces automatic partition creation for RANGE COLUMNS partitioning using an INTERVAL clause. The implementation includes parser updates to support the new syntax, logic within the table opening process to trigger partition creation during DML operations, and internal ALTER TABLE mechanics to append new partitions. The review feedback identifies a critical bug where partitioning fields are not correctly initialized during the auto-creation process, a design limitation where partitions are only created up to the current system time, and the need to support legacy DATE and DATETIME types for broader compatibility.
| Table_specification_st create_info; | ||
| Alter_info alter_info; | ||
| TABLE *table= tl->table; | ||
| /* TODO: this may still trigger MSAN unitialised? */ |
There was a problem hiding this comment.
This TODO should be addressed to ensure that the thread state is correctly initialized and doesn't trigger MSAN warnings during the internal partition creation process.
| if (ftype != MYSQL_TYPE_DATETIME2 && ftype != MYSQL_TYPE_NEWDATE) | ||
| { | ||
| th->partition_field_type_not_allowed(sql_field->field_name); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
The type check for range interval partitioning should also include the legacy MYSQL_TYPE_DATETIME and MYSQL_TYPE_DATE types. While modern MariaDB versions prefer DATETIME2 and NEWDATE, existing tables might still use the older types, and they should be supported for interval partitioning as well.
if (ftype != MYSQL_TYPE_DATETIME2 && ftype != MYSQL_TYPE_NEWDATE &&
ftype != MYSQL_TYPE_DATETIME && ftype != MYSQL_TYPE_DATE)
{
th->partition_field_type_not_allowed(sql_field->field_name);
return 1;
}3 participants
/gemini review