Storages: introduce trim min-max index for DATE/DATETIME/TIMESTAMP rough set filtering#10982
Merged
ti-chi-bot[bot] merged 20 commits intoJul 21, 2026
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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
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
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.
Keep the RSResult matrix on non-nullable indexes and isolate AllNull; replace unsupported cast SQL with a bitand stand-in.
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
ti-chi-bot
Bot
added
release-note
JaySon-Huang
commented
Jul 21, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dbms/src/Storages/tests/gtest_filter_parser.cpp (1)
1068-1129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing
generateRsOperatorvia an injectable attr-creator.This test re-implements ~40 lines of
generateRsOperator's setup (compile → traverse → buildDAGQueryInfo→parseDAGQuery) only to swap in a creator that always returns an emptyAttr.type. Extracting an optionalAttrCreatorByColumnIDparameter on the helper would let this test call it directly and keep the two paths from drifting.🤖 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 1068 - 1129, Refactor the test setup to reuse generateRsOperator instead of duplicating query compilation, executor traversal, DAGQueryInfo construction, and parseDAGQuery calls. Add an optional injectable AttrCreatorByColumnID parameter to generateRsOperator, defaulting to its existing creator, then pass the empty-type creator from MissingAttrTypeBecomesUnsupported while preserving the current unsupported-operator assertions.
🤖 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/tests/gtest_filter_parser.cpp`:
- Around line 1068-1129: Refactor the test setup to reuse generateRsOperator
instead of duplicating query compilation, executor traversal, DAGQueryInfo
construction, and parseDAGQuery calls. Add an optional injectable
AttrCreatorByColumnID parameter to generateRsOperator, defaulting to its
existing creator, then pass the empty-type creator from
MissingAttrTypeBecomesUnsupported while preserving the current
unsupported-operator assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9df83f3e-0993-4f05-989b-d648a826238e
📒 Files selected for processing (20)
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/Not.hdbms/src/Storages/DeltaMerge/Filter/RSOperator.hdbms/src/Storages/DeltaMerge/FilterParser/FilterParser.cppdbms/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 (18)
- dbms/src/Interpreters/Settings.h
- dbms/src/Storages/DeltaMerge/File/DMFileBlockOutputStream.cpp
- dbms/src/Storages/DeltaMerge/Filter/DateQueryDomain.h
- dbms/src/Common/TiFlashMetrics.h
- dbms/src/Storages/DeltaMerge/FilterParser/FilterParser.cpp
- dbms/src/Storages/DeltaMerge/Filter/Equal.h
- dbms/src/Storages/DeltaMerge/File/DMFilePackFilter.h
- dbms/src/Storages/DeltaMerge/File/DMFileWriter.h
- dbms/src/Storages/DeltaMerge/Filter/DateQueryDomain.cpp
- dbms/src/Storages/DeltaMerge/Filter/DateRange.h
- dbms/src/Storages/DeltaMerge/Filter/RSOperator.h
- dbms/src/Storages/DeltaMerge/Filter/In.h
- docs/design/2026-07-14-trim-minmax-for-date-types.md
- dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.cpp
- dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.h
- dbms/src/Storages/DeltaMerge/File/DMFilePackFilter.cpp
- dbms/src/Storages/DeltaMerge/tests/gtest_dm_trim_minmax_index.cpp
- dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp
yudongusa
approved these changes
Jul 21, 2026
close pingcap#10923\n\ntest: cover dropped table schema sync race test: remove flaky alter drop table case tests: use failpoint instead\n\nSigned-off-by: JaySon-Huang <tshent@qq.com>
JinheLin
reviewed
Jul 21, 2026
| M(SettingFloat, dt_bg_gc_delta_delete_ratio_to_trigger_gc, 0.3, "Trigger segment's gc when the ratio of delta delete range to stable exceeds this ratio.") \ | ||
| M(SettingBool, dt_enable_logical_split, false, "Enable logical split or not in DeltaTree Engine.") \ | ||
| M(SettingBool, dt_enable_rough_set_filter, true, "Whether to parse where expression as Rough Set Index filter or not.") \ | ||
| M(SettingBool, dt_enable_trim_minmax, true, "Whether to generate and use trim min-max index for DATE/DATETIME/TIMESTAMP Rough Set filtering.") \ |
Contributor
Author
There was a problem hiding this comment.
Yes. Set default to true for this feature branch after discussed with @windtalker so that user don't need to add extra param when doing POC using this branch.
Contributor
Author
|
/test pull-unit-test |
JinheLin
approved these changes
Jul 21, 2026
ti-chi-bot
Bot
added
lgtm
approved
and removed
needs-1-more-lgtm
Contributor
yongman
approved these changes
Jul 21, 2026
Contributor
|
@yongman: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions 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. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JinheLin, yongman, yudongusa The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Contributor
Author
|
/test pull-unit-test |
ti-chi-bot
Bot
merged commit Jul 21, 2026
1bab99c
into
pingcap:feature/release-8.5-materialized-view
5 checks passed
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
4 participants
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:
Applications sometimes use a far-future temporal value, such as
2100-01-01 00:00:00, as a sentinel for an unsettled record while normalDATE,DATETIME, orTIMESTAMPvalues remain concentrated in a recent range.Although such sentinels are sparse, they pollute the ordinary pack-level min-max index. With 8,192 rows per pack and an independently distributed sentinel probability of
1/10000, about 55.9% of packs contain at least one sentinel. Their ordinary maximum is extended to 2100, so narrow recent-time predicates cannot exclude those packs and must read and filter more data.The ordinary min-max index cannot simply ignore a particular application value because queries may intentionally access it. TiFlash needs an optional temporal index that improves pruning for eligible predicates without changing SQL semantics or weakening the ordinary fallback path.
What is changed and how it works?
This PR adds an optional pack-level trim min-max index for user
DATE,DATETIME, andTIMESTAMPcolumns in DeltaMerge DMFile V3 / MetaV2.The effective interval for format V1 is the half-open range
[1900-01-01 00:00:00, 2099-12-01 00:00:00). The ordinary min-max index remains unchanged and continues to cover every non-NULL, non-deleted value. The trim index covers only values inside the effective interval and records directional per-pack marks when values are trimmed below or above it.Write path and disk format
dt_enable_trim_minmax=true..idxpayload and its byte format unchanged.<column-stream>.trim.idxmerged subfile only when the DMFile contains trimmed outliers.ColumnStat.trim_minmax_index = 105.has_null, bit 1 ishas_trimmed_low, and bit 2 ishas_trimmed_high.Query-domain analysis and read path
Equal,IN, bounded ranges, and one-sided ranges.ANDrange bounds without sharing eligibility acrossORorNOTbranches.TIMESTAMPconstants through the request time zone into UTC-packed values before eligibility checks; compareDATETIMEandDATEusing calendar-value semantics.None -> SomeandAll -> Somewhen trimmed values can invalidate the uncorrected result.Configuration and observability
dt_enable_trim_minmaxsetting, defaulting tofalse. It controls both generation and query use of trim min-max indexes.All/Some/None/AllNullresults, and conservative correction counts.Compatibility
dt_enable_trim_minmaxrequires removing that unknown setting fromtiflash.tomlbefore startup.See the design document for the correctness proof, format details, and rollout constraints.
Check List
Tests
Unit coverage includes:
DATE, fractionalDATETIME, andTIMESTAMPpacked bounds and time-zone conversion;AND/ORtrees, and per-predicate eligibility;RSResultcorrection matrix, trim/ordinary pack-filter selection, missing attribute types, and DMFile MetaV2 persistence/drop behavior.The PR unit-test and integration-test CI jobs pass.
Manual validation used the same generated 500-million-row data set in two tables with different import/DMFile layouts:
test.bc_bet_records_500mandtest.bc_bet_records_500m_stream.All/Some/None,none_to_some, andall_to_some; the disabled run exercised onlyfallback_disabled. See the 500M correctness report.2,125.5 ms -> 1,613.2 ms) on the regular-import table and by 24.9% (2,066.3 ms -> 1,552.7 ms) on the stream-import table. P04 physical scanned rows decreased by 52.4% and 52.6%, respectively. The P05 outside-range control had unchanged scanned rows and no meaningful benefit, as expected. See the 500M benchmark report.5d56de051244ac7c893de5ac029f0013098f95dc, removed the unknowndt_enable_trim_minmaxsetting, and forced both tables through TiFlash MPP. Both 500-million-row invariant scans and complete Q01-Q33 suites passed with zero differences from the TiKV oracle, and no post-start DMFile/index/checksum/corruption error was logged. This validates read compatibility only; it does not cover old-version writes or metadata rewrites. See the downgrade report.The data generator, distributions, import paths, and SQL suites are documented in the dbgen trim min-max example.
Side effects
The feature is disabled by default. When enabled, DMFile writing performs additional min/max comparisons and may persist one optional trim subfile per eligible temporal column with outliers. Eligible reads load the trim index in preference to ordinary min-max; fallback queries retain the ordinary path.
Documentation
Release note
Summary by CodeRabbit
New Features
DATE,DATETIME, andTIMESTAMPdata.IN, and range predicates.Documentation
Tests