Storages: introduce trim min-max index for DATE/DATETIME/TIMESTAMP rough set filtering#10982
Open
JaySon-Huang wants to merge 15 commits into
Conversation
Signed-off-by: JaySon-Huang <tshent@qq.com>
Add ColumnStat field 105, pack-mark accessors, trim subfile naming, and default-off read/write settings so Readers can safely ignore or fall back without changing ordinary min-max behavior.
Build ordinary and trim indexes in one pack scan for V3 MyDate/MyDateTime columns, and persist .trim.idx only when trimmed outliers exist.
Normalize temporal ranges into DateRange, select trim indexes per DMFile stored E, and apply conservative low/high flag corrections in roughCheck.
Contributor
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDeltaMerge adds optional temporal trim min-max indexes for date-like columns, including metadata persistence, DMFile writing and reading, query-domain planning, rough-check correction, configuration propagation, compatibility handling, tests, metrics, and a design specification. ChangesTemporal trim min-max indexing
Estimated code review effort: 5 (Critical) | ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
Contributor
|
[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 |
ti-chi-bot
Bot
added
the
size/XXL
Prevent same-column OR branches from incorrectly sharing a loaded trim index when only some query domains are trim-eligible, avoiding false None pack pruning.
Gate trim range normalization behind dt_enable_trim_minmax_read, keep original operators when bounds cannot be parsed, and never return All for an empty DateRange domain.
Signed-off-by: JaySon-Huang <tshent@qq.com>
JaySon-Huang
marked this pull request as ready for review
July 16, 2026 08:32
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.h (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the prescribed explicit-width type header.
Line 22 should use
Core/Types.hrather than relying on the legacy type header.Proposed fix
-#include <common/types.h> +#include <Core/Types.h>As per coding guidelines, explicit-width types must come from
dbms/src/Core/Types.h.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.h` at line 22, Replace the legacy common types include in TrimMinMaxIndex.h with the prescribed Core/Types.h header, ensuring explicit-width types are sourced from the standard Core type definitions.Source: Coding guidelines
dbms/src/Storages/DeltaMerge/tests/gtest_dm_trim_minmax_index.cpp (1)
784-820: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this test distinguish trim from ordinary filtering.
The current
[2020, 2021]query yieldsSomewith the ordinary min-max index too, soEXPECT_NE(r, All)passes even if trim loading is broken.Use an in-range gap such as
[2050, 2051], then assert:
- trim read disabled:
Some- trim read enabled:
None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/DeltaMerge/tests/gtest_dm_trim_minmax_index.cpp` around lines 784 - 820, The test PackFilterUsesTrimWhenReadEnabled must distinguish trim behavior from ordinary min-max filtering. Change the query range to an in-range gap such as [2050, 2051], add or reuse a scan with enable_trim_minmax_read disabled and assert its pack result is Some, then keep the enabled scan and assert its result is None rather than merely not All.dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp (1)
108-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVariable naming convention mismatch with guidelines.
While the introduced variables align with the historical
snake_casestyle used heavily throughout this repository, the coding guidelines prescribecamelCasefor variables. Consider refactoring the variables in these blocks tocamelCaseto comply with the project standards.
dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp#L108-L147: Renamepack_mark,has_value,allowed_mask,min_idx,max_idx, andtrimmed_maskto camelCase.dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp#L184-L195: Renamepack_markto camelCase.dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp#L214-L217: Renamepack_marksandhas_value_marksto camelCase.dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.h#L35-L38: Renamepack_marks_andhas_value_marks_to camelCase.dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.h#L169-L171: Renamepack_marksandhas_value_marksto camelCase.dbms/src/Storages/DeltaMerge/File/DMFileWriter.h#L106-L110: Renametrim_minmaxes,trim_lower, andtrim_upperto camelCase.dbms/src/Storages/DeltaMerge/File/DMFileWriter.h#L131-L143: Renameenable_trim_minmax_writeto camelCase.dbms/src/Storages/DeltaMerge/File/DMFileWriter.cpp#L116-L124: Renamecan_trimanddo_index_streamto camelCase.dbms/src/Storages/DeltaMerge/File/DMFileWriter.cpp#L133-L139: Renamedo_index_streamto camelCase.dbms/src/Storages/DeltaMerge/File/DMFileWriter.cpp#L224-L239: Renameeffective_del_markto camelCase.dbms/src/Storages/DeltaMerge/File/DMFileWriter.cpp#L366-L395: Renametrim_index_bytesto camelCase.As per coding guidelines:
Method and variable names should use camelCase (e.g., readBlock, totalBytes)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp` around lines 108 - 147, Rename the listed variables and member variables to camelCase while preserving behavior and updating every declaration, definition, reference, and call site: in dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp ranges 108-147, 184-195, and 214-217; dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.h ranges 35-38 and 169-171; dbms/src/Storages/DeltaMerge/File/DMFileWriter.h ranges 106-110 and 131-143; and dbms/src/Storages/DeltaMerge/File/DMFileWriter.cpp ranges 116-124, 133-139, 224-239, and 366-395. Specifically convert pack_mark, has_value, allowed_mask, min_idx, max_idx, trimmed_mask, pack_marks, has_value_marks, pack_marks_, has_value_marks_, trim_minmaxes, trim_lower, trim_upper, enable_trim_minmax_write, can_trim, do_index_stream, effective_del_mark, and trim_index_bytes to camelCase.Source: Coding guidelines
dbms/src/Storages/tests/gtest_filter_parser.cpp (1)
459-460: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
fmt::formatfor string construction.These lines construct queries using string concatenation. As per coding guidelines, C++ code should prefer
fmt::formatfor string construction, similar to the other test cases further down in this file.
dbms/src/Storages/tests/gtest_filter_parser.cpp#L459-L460: Refactor this concatenation to usefmt::format.dbms/src/Storages/tests/gtest_filter_parser.cpp#L484-L485: Refactor this concatenation to usefmt::format.dbms/src/Storages/tests/gtest_filter_parser.cpp#L509-L510: Refactor this concatenation to usefmt::format.♻️ Proposed refactors for all affected sites
For
dbms/src/Storages/tests/gtest_filter_parser.cpp#L459-L460:- const auto query = String("select * from default.t_111 where col_timestamp > cast_string_datetime('") - + datetime + String("')"); + const auto query = fmt::format("select * from default.t_111 where col_timestamp > cast_string_datetime('{}')", datetime);For
dbms/src/Storages/tests/gtest_filter_parser.cpp#L484-L485:- const auto query = String("select * from default.t_111 where col_timestamp > cast_string_datetime('") - + datetime + String("')"); + const auto query = fmt::format("select * from default.t_111 where col_timestamp > cast_string_datetime('{}')", datetime);For
dbms/src/Storages/tests/gtest_filter_parser.cpp#L509-L510:- const auto query = String("select * from default.t_111 where col_timestamp > cast_string_datetime('") - + datetime + String("')"); + const auto query = fmt::format("select * from default.t_111 where col_timestamp > cast_string_datetime('{}')", datetime);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/tests/gtest_filter_parser.cpp` around lines 459 - 460, Replace the query string concatenation in dbms/src/Storages/tests/gtest_filter_parser.cpp at lines 459-460, 484-485, and 509-510 with fmt::format, preserving each query’s existing text and datetime substitution.Source: Coding guidelines
dbms/src/Storages/DeltaMerge/Filter/RSOperator.h (1)
145-155: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid copying elements when appending vectors.
Since
child->getIndexRequests()returnsRSIndexRequestsby value, you can move its elements intoreqsinstead of copying them.RSIndexRequestcontains anstd::optional<DateQueryDomain>, which dynamically allocates itsstd::vector<Field>, so moving avoids unnecessary allocations.♻️ Proposed refactor
RSIndexRequests getIndexRequests() override { RSIndexRequests reqs; for (const auto & child : children) { auto child_reqs = child->getIndexRequests(); - reqs.insert(reqs.end(), child_reqs.begin(), child_reqs.end()); + reqs.insert(reqs.end(), std::make_move_iterator(child_reqs.begin()), std::make_move_iterator(child_reqs.end())); } return reqs; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/DeltaMerge/Filter/RSOperator.h` around lines 145 - 155, Update RSOperator::getIndexRequests to move elements from each temporary child_reqs into reqs when appending, using move-aware range insertion while preserving the existing aggregation behavior.docs/design/2026-07-14-trim-minmax-for-date-types.md (1)
513-513: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEscape pipe characters in Markdown tables.
The
||operator inside the inline code block is interpreted as a table column delimiter by some Markdown parsers and linters, resulting in "Too many cells" rendering issues. Consider usingorinstead to ensure the table renders correctly across all tools.♻️ Proposed refactor
-| Equality / IN / bounded range with `Q ⊆ E` | false | `has_trimmed_low || has_trimmed_high` | +| Equality / IN / bounded range with `Q ⊆ E` | false | `has_trimmed_low or has_trimmed_high` |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/2026-07-14-trim-minmax-for-date-types.md` at line 513, Update the table entry for “Equality / IN / bounded range with Q ⊆ E” to replace the inline-code `||` operator with `or`, preserving the existing condition meaning while preventing Markdown table parsing issues.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@dbms/src/Storages/DeltaMerge/Filter/RSOperator.h`:
- Around line 145-155: Update RSOperator::getIndexRequests to move elements from
each temporary child_reqs into reqs when appending, using move-aware range
insertion while preserving the existing aggregation behavior.
In `@dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp`:
- Around line 108-147: Rename the listed variables and member variables to
camelCase while preserving behavior and updating every declaration, definition,
reference, and call site: in dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp
ranges 108-147, 184-195, and 214-217;
dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.h ranges 35-38 and 169-171;
dbms/src/Storages/DeltaMerge/File/DMFileWriter.h ranges 106-110 and 131-143; and
dbms/src/Storages/DeltaMerge/File/DMFileWriter.cpp ranges 116-124, 133-139,
224-239, and 366-395. Specifically convert pack_mark, has_value, allowed_mask,
min_idx, max_idx, trimmed_mask, pack_marks, has_value_marks, pack_marks_,
has_value_marks_, trim_minmaxes, trim_lower, trim_upper,
enable_trim_minmax_write, can_trim, do_index_stream, effective_del_mark, and
trim_index_bytes to camelCase.
In `@dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.h`:
- Line 22: Replace the legacy common types include in TrimMinMaxIndex.h with the
prescribed Core/Types.h header, ensuring explicit-width types are sourced from
the standard Core type definitions.
In `@dbms/src/Storages/DeltaMerge/tests/gtest_dm_trim_minmax_index.cpp`:
- Around line 784-820: The test PackFilterUsesTrimWhenReadEnabled must
distinguish trim behavior from ordinary min-max filtering. Change the query
range to an in-range gap such as [2050, 2051], add or reuse a scan with
enable_trim_minmax_read disabled and assert its pack result is Some, then keep
the enabled scan and assert its result is None rather than merely not All.
In `@dbms/src/Storages/tests/gtest_filter_parser.cpp`:
- Around line 459-460: Replace the query string concatenation in
dbms/src/Storages/tests/gtest_filter_parser.cpp at lines 459-460, 484-485, and
509-510 with fmt::format, preserving each query’s existing text and datetime
substitution.
In `@docs/design/2026-07-14-trim-minmax-for-date-types.md`:
- Line 513: Update the table entry for “Equality / IN / bounded range with Q ⊆
E” to replace the inline-code `||` operator with `or`, preserving the existing
condition meaning while preventing Markdown table parsing issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 80025a71-60f0-42c1-9527-1cbd354a2b5b
📒 Files selected for processing (39)
dbms/src/Interpreters/Settings.hdbms/src/Storages/DeltaMerge/File/ColumnStat.hdbms/src/Storages/DeltaMerge/File/DMFile.cppdbms/src/Storages/DeltaMerge/File/DMFile.hdbms/src/Storages/DeltaMerge/File/DMFileBlockInputStream.cppdbms/src/Storages/DeltaMerge/File/DMFileBlockInputStream.hdbms/src/Storages/DeltaMerge/File/DMFileBlockOutputStream.cppdbms/src/Storages/DeltaMerge/File/DMFileMeta.cppdbms/src/Storages/DeltaMerge/File/DMFileMetaV2.cppdbms/src/Storages/DeltaMerge/File/DMFileMetaV2.hdbms/src/Storages/DeltaMerge/File/DMFilePackFilter.cppdbms/src/Storages/DeltaMerge/File/DMFilePackFilter.hdbms/src/Storages/DeltaMerge/File/DMFileUtil.cppdbms/src/Storages/DeltaMerge/File/DMFileUtil.hdbms/src/Storages/DeltaMerge/File/DMFileWriter.cppdbms/src/Storages/DeltaMerge/File/DMFileWriter.hdbms/src/Storages/DeltaMerge/Filter/DateQueryDomain.cppdbms/src/Storages/DeltaMerge/Filter/DateQueryDomain.hdbms/src/Storages/DeltaMerge/Filter/DateRange.hdbms/src/Storages/DeltaMerge/Filter/Equal.hdbms/src/Storages/DeltaMerge/Filter/In.hdbms/src/Storages/DeltaMerge/Filter/PushDownFilter.cppdbms/src/Storages/DeltaMerge/Filter/RSOperator.cppdbms/src/Storages/DeltaMerge/Filter/RSOperator.hdbms/src/Storages/DeltaMerge/FilterParser/FilterParser.cppdbms/src/Storages/DeltaMerge/FilterParser/FilterParser.hdbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cppdbms/src/Storages/DeltaMerge/Index/MinMaxIndex.hdbms/src/Storages/DeltaMerge/Index/RSIndex.hdbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.cppdbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.hdbms/src/Storages/DeltaMerge/Segment.cppdbms/src/Storages/DeltaMerge/Segment.hdbms/src/Storages/DeltaMerge/dtpb/dmfile.protodbms/src/Storages/DeltaMerge/tests/gtest_dm_meta_version.cppdbms/src/Storages/DeltaMerge/tests/gtest_dm_trim_minmax_index.cppdbms/src/Storages/StorageDisaggregatedRemote.cppdbms/src/Storages/tests/gtest_filter_parser.cppdocs/design/2026-07-14-trim-minmax-for-date-types.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dbms/src/Storages/DeltaMerge/File/DMFileBlockOutputStream.cpp (1)
29-34: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate
dt_enable_trim_minmaxintoDMFileWriter::Options
DMFileWriter::addStreamsonly builds trim-minmax indexes whenoptions.enable_trim_minmax_writeis true, but this call site leaves it at the defaultfalse. Passcontext.getSettingsRef().dt_enable_trim_minmaxhere so writers actually emit the trim artifacts that readers expect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/DeltaMerge/File/DMFileBlockOutputStream.cpp` around lines 29 - 34, Update the DMFileWriter::Options construction in DMFileBlockOutputStream to pass context.getSettingsRef().dt_enable_trim_minmax as enable_trim_minmax_write, ensuring addStreams creates the expected trim-minmax indexes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@dbms/src/Storages/DeltaMerge/File/DMFileBlockOutputStream.cpp`:
- Around line 29-34: Update the DMFileWriter::Options construction in
DMFileBlockOutputStream to pass context.getSettingsRef().dt_enable_trim_minmax
as enable_trim_minmax_write, ensuring addStreams creates the expected
trim-minmax indexes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fcd98fa9-8ed4-489c-a453-1fd670a76b11
📒 Files selected for processing (12)
dbms/src/Interpreters/Settings.hdbms/src/Storages/DeltaMerge/File/DMFileBlockInputStream.cppdbms/src/Storages/DeltaMerge/File/DMFileBlockInputStream.hdbms/src/Storages/DeltaMerge/File/DMFileBlockOutputStream.cppdbms/src/Storages/DeltaMerge/File/DMFilePackFilter.cppdbms/src/Storages/DeltaMerge/File/DMFilePackFilter.hdbms/src/Storages/DeltaMerge/Filter/PushDownFilter.cppdbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.cppdbms/src/Storages/DeltaMerge/Segment.cppdbms/src/Storages/DeltaMerge/Segment.hdbms/src/Storages/DeltaMerge/tests/gtest_dm_trim_minmax_index.cppdbms/src/Storages/StorageDisaggregatedRemote.cpp
🚧 Files skipped from review as they are similar to previous changes (10)
- dbms/src/Storages/DeltaMerge/Segment.h
- dbms/src/Storages/StorageDisaggregatedRemote.cpp
- dbms/src/Storages/DeltaMerge/File/DMFileBlockInputStream.cpp
- dbms/src/Storages/DeltaMerge/Filter/PushDownFilter.cpp
- dbms/src/Storages/DeltaMerge/File/DMFileBlockInputStream.h
- dbms/src/Storages/DeltaMerge/Segment.cpp
- dbms/src/Storages/DeltaMerge/File/DMFilePackFilter.h
- dbms/src/Storages/DeltaMerge/tests/gtest_dm_trim_minmax_index.cpp
- dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.cpp
- dbms/src/Storages/DeltaMerge/File/DMFilePackFilter.cpp
JaySon-Huang
force-pushed
the
jayson/trim_datetime_minmax_index_mv
branch
from
July 16, 2026 14:55
73caf04 to
3dd7261
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dbms/src/Storages/DeltaMerge/File/DMFileWriter.cpp`:
- Line 392: Update TrimMinMax::makeDefaultProps to persist the supplied pack
count via set_pack_count before returning the default properties. Ensure the
metadata created by the DMFileWriter call includes dmfile->getPacks(), allowing
readers to validate and load non-empty trim indexes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3746cbc8-080b-4371-87a5-3d60a40b1608
📒 Files selected for processing (20)
dbms/src/Interpreters/Settings.hdbms/src/Storages/DeltaMerge/File/ColumnStat.hdbms/src/Storages/DeltaMerge/File/DMFileBlockInputStream.cppdbms/src/Storages/DeltaMerge/File/DMFileBlockInputStream.hdbms/src/Storages/DeltaMerge/File/DMFileBlockOutputStream.cppdbms/src/Storages/DeltaMerge/File/DMFilePackFilter.cppdbms/src/Storages/DeltaMerge/File/DMFilePackFilter.hdbms/src/Storages/DeltaMerge/File/DMFileUtil.cppdbms/src/Storages/DeltaMerge/File/DMFileUtil.hdbms/src/Storages/DeltaMerge/File/DMFileWriter.cppdbms/src/Storages/DeltaMerge/File/DMFileWriter.hdbms/src/Storages/DeltaMerge/Filter/DateRange.hdbms/src/Storages/DeltaMerge/Filter/PushDownFilter.cppdbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.cppdbms/src/Storages/DeltaMerge/Segment.cppdbms/src/Storages/DeltaMerge/Segment.hdbms/src/Storages/DeltaMerge/tests/gtest_dm_meta_version.cppdbms/src/Storages/DeltaMerge/tests/gtest_dm_trim_minmax_index.cppdbms/src/Storages/StorageDisaggregatedRemote.cppdbms/src/Storages/tests/gtest_filter_parser.cpp
🚧 Files skipped from review as they are similar to previous changes (17)
- dbms/src/Storages/DeltaMerge/File/DMFileUtil.cpp
- dbms/src/Storages/DeltaMerge/File/DMFileUtil.h
- dbms/src/Interpreters/Settings.h
- dbms/src/Storages/DeltaMerge/File/DMFileBlockInputStream.cpp
- dbms/src/Storages/StorageDisaggregatedRemote.cpp
- dbms/src/Storages/DeltaMerge/Segment.h
- dbms/src/Storages/DeltaMerge/File/DMFileBlockInputStream.h
- dbms/src/Storages/DeltaMerge/File/DMFileBlockOutputStream.cpp
- dbms/src/Storages/DeltaMerge/File/DMFilePackFilter.h
- dbms/src/Storages/DeltaMerge/Filter/DateRange.h
- dbms/src/Storages/tests/gtest_filter_parser.cpp
- dbms/src/Storages/DeltaMerge/Segment.cpp
- dbms/src/Storages/DeltaMerge/File/DMFilePackFilter.cpp
- dbms/src/Storages/DeltaMerge/tests/gtest_dm_meta_version.cpp
- dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.cpp
- dbms/src/Storages/DeltaMerge/tests/gtest_dm_trim_minmax_index.cpp
- dbms/src/Storages/DeltaMerge/File/ColumnStat.h
| merged_file.file_info.size += trim_index_bytes; | ||
| buffer->next(); | ||
|
|
||
| col_stat.trim_minmax_index = TrimMinMax::makeDefaultProps(*removeNullable(type), dmfile->getPacks()); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist the trim-index pack count in metadata.
TrimMinMax::makeDefaultProps(..., dmfile->getPacks()) currently ignores pack_count because set_pack_count is commented out. The reader validates this metadata against the actual pack count, so non-empty trim indexes will be rejected as MetadataMismatch and never loaded. Persist pack_count in makeDefaultProps before relying on this call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dbms/src/Storages/DeltaMerge/File/DMFileWriter.cpp` at line 392, Update
TrimMinMax::makeDefaultProps to persist the supplied pack count via
set_pack_count before returning the default properties. Ensure the metadata
created by the DMFileWriter call includes dmfile->getPacks(), allowing readers
to validate and load non-empty trim indexes.
Signed-off-by: JaySon-Huang <tshent@qq.com>
JaySon-Huang
force-pushed
the
jayson/trim_datetime_minmax_index_mv
branch
from
July 16, 2026 15:10
3dd7261 to
99b9016
Compare
JaySon-Huang
changed the title
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
dbms/src/Storages/DeltaMerge/tests/gtest_dm_trim_minmax_index.cpp (1)
158-171: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftExercise an actual legacy protobuf reader instead of manually clearing the field.
Using the current generated
dtpb::ColumnStatand callingclear_trim_minmax_index()does not verify that an old reader can parse and rewrite field 105 or tolerate the additional merged subfile. Add a legacy-schema fixture or serialized old-reader round trip; retain this case separately to test fallback after metadata loss.Based on PR objectives, old-reader and old-version metadata-rewrite compatibility must be validated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/DeltaMerge/tests/gtest_dm_trim_minmax_index.cpp` around lines 158 - 171, Replace the manual clear_trim_minmax_index() simulation in this test with a legacy-schema fixture or serialized round trip using a protobuf definition that omits field 105 and the additional merged subfile, then parse and rewrite the bytes before calling ColumnStat::mergeFromProto. Keep this scenario separate from the existing metadata-loss fallback test, which should continue explicitly validating behavior after trim metadata is removed.dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.cpp (1)
282-282: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse explicit width type
UInt8.For consistency with
pack_marks(PaddedPODArray<UInt8>) and to adhere to the coding guidelines, useUInt8instead ofunsigned char.♻️ Proposed refactor
- for (unsigned char pack_mark : pack_marks) + for (UInt8 pack_mark : pack_marks)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.cpp` at line 282, Update the range-based loop over pack_marks in the surrounding index logic to declare pack_mark as UInt8 instead of unsigned char, preserving the existing iteration behavior and matching pack_marks’ element type.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/2026-07-14-trim-minmax-for-date-types.md`:
- Around line 607-615: Replace the single dt_enable_trim_minmax setting with
independent write and read rollout controls: one governing trim-index generation
by writers and one governing trim metadata/subfile consumption by readers.
Update the documented enabled/disabled behavior and rollout sequence to support
write-first/read-later deployment and independent read rollback, including the
corresponding PreferTrim temporal-range normalization behavior.
---
Nitpick comments:
In `@dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.cpp`:
- Line 282: Update the range-based loop over pack_marks in the surrounding index
logic to declare pack_mark as UInt8 instead of unsigned char, preserving the
existing iteration behavior and matching pack_marks’ element type.
In `@dbms/src/Storages/DeltaMerge/tests/gtest_dm_trim_minmax_index.cpp`:
- Around line 158-171: Replace the manual clear_trim_minmax_index() simulation
in this test with a legacy-schema fixture or serialized round trip using a
protobuf definition that omits field 105 and the additional merged subfile, then
parse and rewrite the bytes before calling ColumnStat::mergeFromProto. Keep this
scenario separate from the existing metadata-loss fallback test, which should
continue explicitly validating behavior after trim metadata is removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 171ba54e-0792-44ee-858d-ecadc1fe5944
📒 Files selected for processing (18)
dbms/src/Common/TiFlashMetrics.hdbms/src/Interpreters/Settings.hdbms/src/Storages/DeltaMerge/File/DMFileBlockOutputStream.cppdbms/src/Storages/DeltaMerge/File/DMFilePackFilter.cppdbms/src/Storages/DeltaMerge/File/DMFilePackFilter.hdbms/src/Storages/DeltaMerge/File/DMFileWriter.hdbms/src/Storages/DeltaMerge/Filter/DateQueryDomain.cppdbms/src/Storages/DeltaMerge/Filter/DateQueryDomain.hdbms/src/Storages/DeltaMerge/Filter/DateRange.hdbms/src/Storages/DeltaMerge/Filter/Equal.hdbms/src/Storages/DeltaMerge/Filter/In.hdbms/src/Storages/DeltaMerge/Filter/RSOperator.hdbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cppdbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.cppdbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.hdbms/src/Storages/DeltaMerge/tests/gtest_dm_trim_minmax_index.cppdbms/src/Storages/tests/gtest_filter_parser.cppdocs/design/2026-07-14-trim-minmax-for-date-types.md
🚧 Files skipped from review as they are similar to previous changes (12)
- dbms/src/Storages/DeltaMerge/File/DMFileWriter.h
- dbms/src/Storages/DeltaMerge/Filter/Equal.h
- dbms/src/Storages/DeltaMerge/File/DMFilePackFilter.h
- dbms/src/Storages/DeltaMerge/Filter/DateQueryDomain.h
- dbms/src/Interpreters/Settings.h
- dbms/src/Storages/DeltaMerge/Filter/DateRange.h
- dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.h
- dbms/src/Storages/DeltaMerge/Filter/In.h
- dbms/src/Storages/DeltaMerge/Filter/DateQueryDomain.cpp
- dbms/src/Storages/DeltaMerge/Filter/RSOperator.h
- dbms/src/Storages/DeltaMerge/File/DMFilePackFilter.cpp
- dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp
Convert missing column types to Unsupported at parse time, and guard getIndexRequests/getRSIndex against empty Attr.type.
Signed-off-by: JaySon-Huang <tshent@qq.com>
…policy. Cover NULL/delete/empty packs and DATE/DATETIME/TIMESTAMP temporal cases; document that invalid MergedSubFileInfo must throw rather than soft-fallback.
Unify DateRange/Equal/In on shared packs and cover AllNull for {NULL,2021}.
Cover != / NOT / OR / CAST with no PreferTrim, and out-of-E ranges that stay ineligible.
Contributor
|
@JaySon-Huang: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #10989
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Summary by CodeRabbit
New Features
DATE,DATETIME, andTIMESTAMPto improve rough-set filtering.dt_enable_trim_minmaxsetting (defaultfalse) to control trim min-max generation and usage.Bug Fixes
.idxfiles aren’t treated as standard min-max indexes.Tests
Documentation