Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces the bank4 workload, which is optimized for partition-heavy DDL stress testing with a schema that includes 126 monthly partitions. It also refactors the DDLRunner to schedule DDL tasks evenly throughout the minute, improving the consistency of the workload. Review feedback points out that the bank4 DDL generator currently only supports TRUNCATE TABLE, which limits the effectiveness of mixed DDL configurations. Additionally, a performance optimization was suggested for the INSERT statement generation to reduce memory allocations by using fmt.Fprintf instead of fmt.Sprintf inside loops.
| func (c *BankWorkload) BuildDDLSql(opts schema.DDLOption) string { | ||
| tableName := getBankTableName(opts.TableIndex) | ||
| return fmt.Sprintf("truncate table %s;", tableName) | ||
| } |
There was a problem hiding this comment.
The BuildDDLSql implementation always returns a TRUNCATE TABLE statement, regardless of the DDL type intended by the scheduler (e.g., add_column, add_index). This makes the rate_per_minute configuration for other DDL types in the provided examples (like ddl_partition_table_mixed.toml) misleading, as they will all result in table truncations. Consider updating the DDLWorkload interface to include the DDL type or implementing logic to vary the DDL statement generated here.
| buf.WriteString(fmt.Sprintf( | ||
| "('%s','%s','%s','%s','%s','%s','%s','%s','%s',%d,%d,'%s',%d,'%s','%s','%s','%s','%s','%s','%s','%s','%s',%d,'%s','%s','%s','%s',%d,%d,%d)", | ||
| col1Value, | ||
| col2Value, | ||
| col3Value, | ||
| col4Value, | ||
| col5Value, | ||
| col6Value, | ||
| col7Value, | ||
| col8Value, | ||
| col9Value, | ||
| col10Value, | ||
| col11Value, | ||
| col12Value, | ||
| col13Value, | ||
| col14Value, | ||
| col15Value, | ||
| col16Value, | ||
| col17Value, | ||
| col18Value, | ||
| col19Value, | ||
| col20Value, | ||
| col21Value, | ||
| col22Value, | ||
| col23Value, | ||
| col24Value, | ||
| col25Value, | ||
| col26Value, | ||
| col27Value, | ||
| col28Value, | ||
| col29Value, | ||
| col30Value, | ||
| )) |
There was a problem hiding this comment.
Using fmt.Sprintf inside a loop to build a large string can be inefficient due to multiple allocations. It is better to use fmt.Fprintf to write directly into the bytes.Buffer.
fmt.Fprintf(&buf, "('%s','%s','%s','%s','%s','%s','%s','%s','%s',%d,%d,'%s',%d,'%s','%s','%s','%s','%s','%s','%s','%s','%s',%d,'%s','%s','%s','%s',%d,%d,%d)",
col1Value, col2Value, col3Value, col4Value, col5Value, col6Value, col7Value, col8Value, col9Value, col10Value,
col11Value, col12Value, col13Value, col14Value, col15Value, col16Value, col17Value, col18Value, col19Value, col20Value,
col21Value, col22Value, col23Value, col24Value, col25Value, col26Value, col27Value, col28Value, col29Value, col30Value)|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
What problem does this PR solve?
Issue Number: close #xxx
What is changed and how it works?
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note