Storages: introduce trim min-max index for DATE/DATETIME/TIMESTAMP rough set filtering - #10981
Open
JaySon-Huang wants to merge 7 commits into
Open
Storages: introduce trim min-max index for DATE/DATETIME/TIMESTAMP rough set filtering#10981JaySon-Huang wants to merge 7 commits into
JaySon-Huang wants to merge 7 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/needs-linked-issue
do-not-merge/work-in-progress
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 |
📝 WalkthroughWalkthroughThis PR adds optional trim min-max indexes for temporal columns. It persists trim metadata and subfiles, normalizes eligible predicates, selects trim indexes during reads, corrects rough-check results, and falls back to ordinary indexes when required. ChangesTemporal trim min-max filtering
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Query
participant FilterParser
participant DMFilePackFilter
participant TrimMinMaxIndex
participant OrdinaryMinMaxIndex
Query->>FilterParser: parse temporal predicates
FilterParser->>DMFilePackFilter: provide trim-preferred index requests
DMFilePackFilter->>TrimMinMaxIndex: validate and load trim payload
alt Trim index is eligible
TrimMinMaxIndex-->>DMFilePackFilter: return trim pack marks and bounds
else Trim index is unavailable or ineligible
DMFilePackFilter->>OrdinaryMinMaxIndex: load ordinary index
OrdinaryMinMaxIndex-->>DMFilePackFilter: return ordinary rough-check data
end
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 |
ti-chi-bot
Bot
added
the
size/XXL
JaySon-Huang
force-pushed
the
jayson/trim_datetime_minmax_index
branch
from
July 22, 2026 07:58
dd5bd76 to
d142197
Compare
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.
JaySon-Huang
force-pushed
the
jayson/trim_datetime_minmax_index
branch
from
July 22, 2026 08:49
d142197 to
0a5609f
Compare
ti-chi-bot
Bot
added
release-note
JaySon-Huang
changed the title
JaySon-Huang
commented
Jul 22, 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, false, "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.
default to false in master branch
Record trim min-max metrics only on Query reads after the cherry-pick left an undeclared read_tag reference on this branch's load() API.
JaySon-Huang
marked this pull request as ready for review
August 10, 2026 06:31
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp (2)
141-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
UInt8for pack-mark loop variables.
dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp#L141-L141: Replaceunsigned char pack_markwithUInt8 packMark.dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.cpp#L285-L285: Replaceunsigned char pack_markwithUInt8 packMark.As per coding guidelines, use explicit width types 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/MinMaxIndex.cpp` at line 141, Replace the pack-mark loop variable with the explicit UInt8 type and packMark naming in MinMaxIndex.cpp:141-141 and TrimMinMaxIndex.cpp:285-285, ensuring the relevant code uses the UInt8 definition from Core/Types.h.Source: Coding guidelines
117-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
DB::Exceptionfor pack validation errors.Lines 117-122 add
RUNTIME_CHECKfailure paths. Replace them withthrow Exception(ErrorCodes::<code>, "… {}", …);. Use an error code declared indbms/src/Common/ErrorCodes.cppanderrors.toml.As per coding guidelines, use
DB::Exceptionfor error handling with the fmt-style constructor.🤖 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 117 - 122, Replace the pack validation RUNTIME_CHECK calls in the MinMaxIndex validation flow with DB::Exception using the fmt-style constructor. Use an existing error code declared in ErrorCodes.cpp and errors.toml, preserve the invalid pack_mark and missing-column conditions, and include the relevant values in each exception message.Source: Coding guidelines
dbms/src/Storages/DeltaMerge/File/ColumnStat.h (1)
49-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftUse camelCase for new C++ identifiers.
dbms/src/Storages/DeltaMerge/File/ColumnStat.h#L49-L50: Renametrim_minmax_indexto a camelCase C++ member. Keep the generated protobuf accessor unchanged.dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.h#L43-L174: Rename new fields and parameters such aspack_mark,format_version, andexpected_pack_count.dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.h#L35-L171: Rename new fields and parameters such aspack_marks,has_value, andallowed_mask.dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp#L109-L147: Rename the matching implementation identifiers.dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.cpp#L100-L291: Rename new local variables and parameters to camelCase.dbms/src/Storages/DeltaMerge/File/DMFileWriter.h#L120-L158: Renametrim_minmaxes,trim_lower,trim_upper, andenable_trim_minmax.As per coding guidelines, method and variable names should use camelCase.
🤖 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/ColumnStat.h` around lines 49 - 50, Rename the newly introduced C++ fields, parameters, and locals to camelCase while preserving generated protobuf accessor names: update trim_minmax_index in dbms/src/Storages/DeltaMerge/File/ColumnStat.h (lines 49-50), identifiers including pack_mark, format_version, and expected_pack_count in dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.h (lines 43-174), pack_marks, has_value, and allowed_mask in dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.h (lines 35-171), their matching implementations in dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp (lines 109-147) and dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.cpp (lines 100-291), and trim_minmaxes, trim_lower, trim_upper, and enable_trim_minmax in dbms/src/Storages/DeltaMerge/File/DMFileWriter.h (lines 120-158). Update all references consistently.Source: Coding guidelines
docs/design/2026-07-14-trim-minmax-for-date-types.md (1)
343-393: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the sketch and the field-number wording with the implementation.
Two documentation accuracy gaps:
- The sketch at lines 346-355 names the wrapper
TrimMinMaxIndexwith a singleminmaxmember. The implementation usesTrimRSIndexwithtype,minmax, andmetamembers, as shown indbms/src/Storages/DeltaMerge/Index/tests/gtest_dm_trim_minmax_index.cpplines 1036-1043.- Line 393 states the field number will be assigned during implementation. Lines 371, 671, 724, and 973 state field 105 definitively. Remove the deferral sentence now that 105 is fixed.
🤖 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` around lines 343 - 393, Update the documentation sketch to use the implemented TrimRSIndex name and reflect its type, minmax, and meta members instead of the simplified TrimMinMaxIndex shape. Also replace the statement that the protobuf field number will be assigned during implementation with a definitive reference to field 105, keeping the existing ColumnStat.trim_minmax_index contract unchanged.dbms/src/Storages/tests/gtest_filter_parser.cpp (1)
788-791: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the timezone after the test mutates it.
Line 791 sets the context timezone to
America/Chicagoand never restores it. IfTiFlashTestEnv::getContext()returns a shared context, the setting leaks into later tests in the same binary. GoogleTest does not guarantee test order, so this creates an ordering dependency. Save the previous timezone and restore it at the end of the test, or reset it inTearDown.♻️ Proposed restore using a scope guard
const auto & time_zone_utc = DateLUT::instance("UTC"); auto ctx = TiFlashTestEnv::getContext(); auto & timezone_info = ctx->getTimezoneInfo(); + const TimezoneInfo saved_timezone_info = timezone_info; + SCOPE_EXIT({ timezone_info = saved_timezone_info; }); timezone_info.resetByTimezoneName("America/Chicago");🤖 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 788 - 791, Update the test that uses timezone_info.resetByTimezoneName to capture the existing timezone before switching to America/Chicago, then restore that timezone when the test exits, including early-return or failure paths; use a scope guard or equivalent cleanup tied to the test’s lifetime.dbms/src/Storages/DeltaMerge/Filter/DateQueryDomain.cpp (1)
362-384: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the emitted
DateRangeorder deterministic.
boundsis astd::unordered_map<ColId, BoundAccumulator>. The loop appends oneDateRangeper column in unspecified order. When a query has two or more temporal columns, the resultingAndchildren order can vary between runs and between builds. This changestoDebugString()andtoJSONObject()output, which the filter-parser tests compare as text. Use an ordered container, or sort bycol_idbefore appending.♻️ Proposed fix
- std::unordered_map<ColId, BoundAccumulator> bounds; + std::map<ColId, BoundAccumulator> bounds;Then remove the now-unneeded include of
<unordered_map>and add<map>.🤖 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/DateQueryDomain.cpp` around lines 362 - 384, Make the iteration over bounds deterministic by replacing the unordered container with an ordered container keyed by ColId, preserving the existing DateRange construction logic in the bounds loop. Remove the unused unordered_map include and add the map include.dbms/src/Storages/DeltaMerge/File/DMFileBlockInputStream.h (1)
237-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
enable_trim_minmaxout of the clean-read field group.Line 241 places
enable_trim_minmaxunder the// clean readcomment, next toenable_handle_clean_read,is_fast_scan, andenable_del_clean_read. The flag controls trim min-max index selection, not clean read. Place it with the other filter-related fields, or give it its own comment.♻️ Proposed fix
// clean read bool enable_handle_clean_read = false; bool is_fast_scan = false; bool enable_del_clean_read = false; - bool enable_trim_minmax = false; UInt64 max_data_version = std::numeric_limits<UInt64>::max(); + // trim min-max index selection + bool enable_trim_minmax = false; // packs filter (filter by pack index) IdSetPtr read_packs;🤖 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/DMFileBlockInputStream.h` around lines 237 - 241, Move the enable_trim_minmax field out of the // clean read group and place it with the other filter-related fields, or add a separate comment identifying its trim min-max index selection purpose. Keep the clean-read group limited to enable_handle_clean_read, is_fast_scan, and enable_del_clean_read.dbms/src/Storages/DeltaMerge/Segment.cpp (1)
1035-1045: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTag query-driven pack-filter loads as
Query.
ReadMode::FastandReadMode::Bitmapdispatch throughgetBitmapFilterInputStreamand applyexecutor->rs_operator; mapping both toReadTag::MVCCmakesrecord_trim_metricsstay false inDMFilePackFilter::load, so trim-selection, fallback, and rough-check metrics remain blind. UseReadTag::Queryfor query paths and keepReadTag::MVCCfor internal/bitmap build paths.🤖 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/Segment.cpp` around lines 1035 - 1045, The pack-filter tag selection in the surrounding read flow must classify query-driven paths as ReadTag::Query, including ReadMode::Fast and ReadMode::Bitmap when they use executor->rs_operator through getBitmapFilterInputStream. Reserve ReadTag::MVCC for internal bitmap-building paths, and preserve the existing DMFilePackFilter::loadFrom arguments.
🤖 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/DMFilePackFilter.h`:
- Around line 211-212: Add direct includes for TrimMinMaxIndex.h and
DateQueryDomain.h in DMFilePackFilter.h so RSIndexRequest, DateQueryDomain, and
TrimMinMaxFallbackReason are visible where tryLoadIndexByRequest() and
tryLoadTrimIndex() are declared; use an existing transitive header only if it
reliably provides all required types.
In `@docs/design/2026-07-14-trim-minmax-for-date-types.md`:
- Around line 572-576: Update the equality/IN/bounded-range row in the
predicate-type table to escape both pipe characters in the
trimmed_nonmatch_exists condition, preserving the displayed logical OR
expression while keeping the row at three cells.
---
Nitpick comments:
In `@dbms/src/Storages/DeltaMerge/File/ColumnStat.h`:
- Around line 49-50: Rename the newly introduced C++ fields, parameters, and
locals to camelCase while preserving generated protobuf accessor names: update
trim_minmax_index in dbms/src/Storages/DeltaMerge/File/ColumnStat.h (lines
49-50), identifiers including pack_mark, format_version, and expected_pack_count
in dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.h (lines 43-174),
pack_marks, has_value, and allowed_mask in
dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.h (lines 35-171), their matching
implementations in dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp (lines
109-147) and dbms/src/Storages/DeltaMerge/Index/TrimMinMaxIndex.cpp (lines
100-291), and trim_minmaxes, trim_lower, trim_upper, and enable_trim_minmax in
dbms/src/Storages/DeltaMerge/File/DMFileWriter.h (lines 120-158). Update all
references consistently.
In `@dbms/src/Storages/DeltaMerge/File/DMFileBlockInputStream.h`:
- Around line 237-241: Move the enable_trim_minmax field out of the // clean
read group and place it with the other filter-related fields, or add a separate
comment identifying its trim min-max index selection purpose. Keep the
clean-read group limited to enable_handle_clean_read, is_fast_scan, and
enable_del_clean_read.
In `@dbms/src/Storages/DeltaMerge/Filter/DateQueryDomain.cpp`:
- Around line 362-384: Make the iteration over bounds deterministic by replacing
the unordered container with an ordered container keyed by ColId, preserving the
existing DateRange construction logic in the bounds loop. Remove the unused
unordered_map include and add the map include.
In `@dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp`:
- Line 141: Replace the pack-mark loop variable with the explicit UInt8 type and
packMark naming in MinMaxIndex.cpp:141-141 and TrimMinMaxIndex.cpp:285-285,
ensuring the relevant code uses the UInt8 definition from Core/Types.h.
- Around line 117-122: Replace the pack validation RUNTIME_CHECK calls in the
MinMaxIndex validation flow with DB::Exception using the fmt-style constructor.
Use an existing error code declared in ErrorCodes.cpp and errors.toml, preserve
the invalid pack_mark and missing-column conditions, and include the relevant
values in each exception message.
In `@dbms/src/Storages/DeltaMerge/Segment.cpp`:
- Around line 1035-1045: The pack-filter tag selection in the surrounding read
flow must classify query-driven paths as ReadTag::Query, including
ReadMode::Fast and ReadMode::Bitmap when they use executor->rs_operator through
getBitmapFilterInputStream. Reserve ReadTag::MVCC for internal bitmap-building
paths, and preserve the existing DMFilePackFilter::loadFrom arguments.
In `@dbms/src/Storages/tests/gtest_filter_parser.cpp`:
- Around line 788-791: Update the test that uses
timezone_info.resetByTimezoneName to capture the existing timezone before
switching to America/Chicago, then restore that timezone when the test exits,
including early-return or failure paths; use a scope guard or equivalent cleanup
tied to the test’s lifetime.
In `@docs/design/2026-07-14-trim-minmax-for-date-types.md`:
- Around line 343-393: Update the documentation sketch to use the implemented
TrimRSIndex name and reflect its type, minmax, and meta members instead of the
simplified TrimMinMaxIndex shape. Also replace the statement that the protobuf
field number will be assigned during implementation with a definitive reference
to field 105, keeping the existing ColumnStat.trim_minmax_index contract
unchanged.
🪄 Autofix
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 Plus
Run ID: 88f47dac-ecc1-448d-b507-4aec7f839ce9
📒 Files selected for processing (40)
dbms/src/Common/TiFlashMetrics.hdbms/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/File/tests/gtest_dm_meta_version.cppdbms/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/PushDownExecutor.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/Index/tests/gtest_dm_trim_minmax_index.cppdbms/src/Storages/DeltaMerge/Segment.cppdbms/src/Storages/DeltaMerge/dtpb/dmfile.protodbms/src/Storages/StorageDisaggregatedRemote.cppdbms/src/Storages/tests/gtest_filter_parser.cppdocs/design/2026-07-14-trim-minmax-for-date-types.md
Comment on lines
+211
to
+212
| void tryLoadIndexByRequest(RSCheckParam & param, const RSIndexRequest & request); | ||
| TrimMinMaxFallbackReason tryLoadTrimIndex(RSCheckParam & param, ColId col_id, const DateQueryDomain & query_domain); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the include list of DMFilePackFilter.h for the trim-index and date-domain declarations.
set -euo pipefail
fd -t f 'DMFilePackFilter.h' dbms/src/Storages/DeltaMerge/File --exec rg -n '^`#include`' {}
fd -t f 'RSOperator.h' dbms/src/Storages/DeltaMerge/Filter --exec rg -n '^`#include`' {}Repository: pingcap/tiflash
Length of output: 1101
🏁 Script executed:
#!/bin/bash
# Description: Map and inspect declarations/includes for DMFilePackFilter.h and header inclusion chains.
set -euo pipefail
echo "== DMFilePackFilter.h outline =="
ast-grep outline dbms/src/Storages/DeltaMerge/File/DMFilePackFilter.h --view compact || true
echo "== DMFilePackFilter.h relevant lines =="
sed -n '1,250p' dbms/src/Storages/DeltaMerge/File/DMFilePackFilter.h
echo "== Header declarations =="
rg -n "class\\s+DateQueryDomain\\b|class\\s+TrimMinMaxFallbackReason\\b|enum class\\s+DateQueryDomain\\b|enum class\\s+TrimMinMaxFallbackReason\\b|struct\\s+DateQueryDomain\\b|struct\\s+TrimMinMaxFallbackReason\\b" dbms/src/Storages/DeltaMerge/Index dbms/src/Storages/DeltaMerge/Filter dbms/src/Storages/DeltaMerge/File -g '*.h' -g '*.hpp' | head -n 200
echo "== direct includes with RSIndexTrim/DateQueryDomain =="
fd -t f 'DMFilePackFilter_fwd.h|RSOperator_fwd.h|ReadMode.h|TrimMinMaxIndex.h|DateQueryDomain.h|ReadMode.h|RSIndex.h|DMFilePackFilterResult.h' dbms/src/Storages/DeltaMerge -x sh -c 'echo "--- $1"; sed -n "1,180p" "$1"' sh {}Repository: pingcap/tiflash
Length of output: 26287
Add includes for the new header types.
ReadMode.h only defines read modes, so tryLoadIndexByRequest() and tryLoadTrimIndex() are declared in DMFilePackFilter.h without RSIndexRequest, DateQueryDomain, or TrimMinMaxFallbackReason visible. Add direct includes for Storages/DeltaMerge/Index/TrimMinMaxIndex.h and Storages/DeltaMerge/Filter/DateQueryDomain.h, or include a header that itself includes them.
🤖 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/DMFilePackFilter.h` around lines 211 - 212,
Add direct includes for TrimMinMaxIndex.h and DateQueryDomain.h in
DMFilePackFilter.h so RSIndexRequest, DateQueryDomain, and
TrimMinMaxFallbackReason are visible where tryLoadIndexByRequest() and
tryLoadTrimIndex() are declared; use an existing transitive header only if it
reliably provides all required types.
Comment on lines
+572
to
+576
| | Predicate type | `trimmed_match_exists` | `trimmed_nonmatch_exists` | | ||
| | --- | --- | --- | | ||
| | Equality / IN / bounded range with `Q ⊆ E` | false | `has_trimmed_low || has_trimmed_high` | | ||
| | Lower-bounded range with bound in `E` | `has_trimmed_high` | `has_trimmed_low` | | ||
| | Upper-bounded range with bound in `E` | `has_trimmed_low` | `has_trimmed_high` | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the pipes in the table cell.
Line 574 contains || inside a table cell. Markdown parses each | as a cell separator, so the row produces five cells against a three-column header. The rendered table drops the condition text. markdownlint reports this as MD056.
Escape both pipes.
🐛 Proposed fix for the broken table row
| Predicate type | `trimmed_match_exists` | `trimmed_nonmatch_exists` |
| --- | --- | --- |
-| 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 \|\| has_trimmed_high` |
| Lower-bounded range with bound in `E` | `has_trimmed_high` | `has_trimmed_low` |
| Upper-bounded range with bound in `E` | `has_trimmed_low` | `has_trimmed_high` |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | Predicate type | `trimmed_match_exists` | `trimmed_nonmatch_exists` | | |
| | --- | --- | --- | | |
| | Equality / IN / bounded range with `Q ⊆ E` | false | `has_trimmed_low || has_trimmed_high` | | |
| | Lower-bounded range with bound in `E` | `has_trimmed_high` | `has_trimmed_low` | | |
| | Upper-bounded range with bound in `E` | `has_trimmed_low` | `has_trimmed_high` | | |
| | Predicate type | `trimmed_match_exists` | `trimmed_nonmatch_exists` | | |
| | --- | --- | --- | | |
| | Equality / IN / bounded range with `Q ⊆ E` | false | `has_trimmed_low \|\| has_trimmed_high` | | |
| | Lower-bounded range with bound in `E` | `has_trimmed_high` | `has_trimmed_low` | | |
| | Upper-bounded range with bound in `E` | `has_trimmed_low` | `has_trimmed_high` | |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 574-574: Table column count
Expected: 3; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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` around lines 572 - 576,
Update the equality/IN/bounded-range row in the predicate-type table to escape
both pipe characters in the trimmed_nonmatch_exists condition, preserving the
displayed logical OR expression while keeping the row at three cells.
Source: Linters/SAST tools
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.
backport #10982 to master branch
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
DATE,DATETIME, andTIMESTAMPcolumns.IN, and range predicates, to use optimized index data.dt_enable_trim_minmaxsetting, disabled by default.