Support decimal rough set minmax filter - #11020
Conversation
ti-chi-bot
Bot
added
release-note
|
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:
📝 WalkthroughWalkthroughThe PR adds decimal support to DMFile min-max index creation and rough-set predicate evaluation. It also updates parser classification, comparison logic, tests, and the design note. ChangesDecimal min-max filtering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FilterParser
participant DMFileWriter
participant MinMaxIndex
participant ValueComparison
FilterParser->>MinMaxIndex: accept TypeDecimal and TypeNewDecimal
DMFileWriter->>MinMaxIndex: create decimal min-max index
MinMaxIndex->>MinMaxIndex: read decimal data through ColumnDecimal
MinMaxIndex->>ValueComparison: compare decimal predicate operands
ValueComparison->>ValueComparison: apply DecimalComparison with operand scales
Suggested reviewers: 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 |
![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 (1)
dbms/src/Storages/DeltaMerge/tests/gtest_dm_minmax_index.cpp (1)
309-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
std::string_viewfor read-only text parameters.Change
getDecimalFieldinputsandcheck_resultsinputtest_casetostd::string_view. Verify thatReadBufferFromStringaccepts a view, or adapt only the buffer boundary if it does not.Also applies to: 2307-2313
🤖 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_minmax_index.cpp` around lines 309 - 315, Update getDecimalField and the check_results test_case parameter to use std::string_view for read-only text inputs. Preserve existing parsing and result-checking behavior, adapting only the ReadBufferFromString boundary if it does not accept a view directly.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/note/rough_set_filter_support_decimal.md`:
- Around line 86-88: Correct the misspelled ValueComparision type to
ValueComparison in every occurrence of the documented comparison calls,
including the referenced sections, so they match the ValueComparison definition.
- Around line 276-286: Correct the focused gtest command in the validation
documentation by removing the nonexistent MinMaxIndexTest.CheckNullEqual filter
and adding MinMaxIndexWriteTest.WriteDecimalMinMaxIndex, while preserving the
other focused test filters.
---
Nitpick comments:
In `@dbms/src/Storages/DeltaMerge/tests/gtest_dm_minmax_index.cpp`:
- Around line 309-315: Update getDecimalField and the check_results test_case
parameter to use std::string_view for read-only text inputs. Preserve existing
parsing and result-checking behavior, adapting only the ReadBufferFromString
boundary if it does not accept a view directly.
🪄 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: 76b07d07-0f8d-491c-a792-caa7b50a4537
📒 Files selected for processing (5)
dbms/src/Storages/DeltaMerge/File/DMFileWriter.cppdbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cppdbms/src/Storages/DeltaMerge/Index/ValueComparison.hdbms/src/Storages/DeltaMerge/tests/gtest_dm_minmax_index.cppdocs/note/rough_set_filter_support_decimal.md
| ```cpp | ||
| ValueComparision<Op>::compare(left_field, type, right_value) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the ValueComparison type name.
Replace ValueComparision with ValueComparison in each occurrence. The current spelling does not match dbms/src/Storages/DeltaMerge/Index/ValueComparison.h.
Also applies to: 154-156, 205-207, 266-270
🤖 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/note/rough_set_filter_support_decimal.md` around lines 86 - 88, Correct
the misspelled ValueComparision type to ValueComparison in every occurrence of
the documented comparison calls, including the referenced sections, so they
match the ValueComparison definition.
Co-authored-by: JaySon <tshent@qq.com>
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
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 (2)
dbms/src/Storages/DeltaMerge/tests/gtest_dm_minmax_index.cpp (2)
66-116: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename the new variables to
camelCase.The new fixture uses
decimal_col_id,parent_path,file_provider,dm_file,column_data,del_mark,col_data,del_mark_col,check_results, andtest_case. Rename them todecimalColId,parentPath,fileProvider,dmFile,columnData,delMark,colData,delMarkColumn,checkResults, andtestCase.As per coding guidelines, method and variable names should use
camelCase.Proposed rename
- static constexpr ColId decimal_col_id = 100; + static constexpr ColId decimalColId = 100; - String parent_path; - FileProviderPtr file_provider; + String parentPath; + FileProviderPtr fileProvider; - std::vector<std::optional<String>> column_data; - std::vector<UInt64> del_mark; + std::vector<std::optional<String>> columnData; + std::vector<UInt64> delMark; - auto col_data = createColumn<Nullable<Decimal64>>(...); - auto del_mark_col = createColumn<UInt8>(...); + auto colData = createColumn<Nullable<Decimal64>>(...); + auto delMarkColumn = createColumn<UInt8>(...);Update all references to the renamed identifiers.
Also applies to: 2277-2315
🤖 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_minmax_index.cpp` around lines 66 - 116, Rename the newly introduced snake_case variables throughout the MinMaxIndexWriteTest fixture and its related test code to the specified camelCase names: decimalColId, parentPath, fileProvider, dmFile, columnData, delMark, colData, delMarkColumn, checkResults, and testCase. Update every declaration and reference consistently, including the additional affected test section.Source: Coding guidelines
1989-2013: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate the restored index contents, not only its size.
WriteDecimalMinMaxIndexchecks thatindex_bytesand the merged sub-file size are non-zero. These checks do not prove that Decimal min/max values deserialize correctly or that the restored index can answer predicates. AfterDMFile::restore, run a known decimal match and non-match through the same read path used byDMFilePackFilter, or inspect the restoredMinMaxIndexdirectly.🤖 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_minmax_index.cpp` around lines 1989 - 2013, The WriteDecimalMinMaxIndex test must validate restored Decimal min/max index behavior, not just non-zero sizes. After DMFile::restore, use the existing DMFilePackFilter read path or inspect the restored MinMaxIndex to verify a known matching decimal predicate is accepted and a known non-matching predicate is rejected, while preserving the current size assertions.
🧹 Nitpick comments (2)
dbms/src/Storages/DeltaMerge/tests/gtest_dm_minmax_index.cpp (2)
274-287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
isNullableDateTypeto match its scope.The helper now covers nullable Int64, date/time types, and Decimal64. Rename it to
isNullableTypeand update its call sites.🤖 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_minmax_index.cpp` around lines 274 - 287, Rename the helper isNullableDateType to isNullableType to reflect that it handles all nullable test datatypes, including Int64, date/time, and Decimal64. Update every call site and declaration consistently, without changing its switch logic or return behavior.
2283-2387: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd negative decimal coverage.
Every Decimal value and threshold in
CheckDecimalis non-negative. Add a negative pack and scale-mismatched equality and range checks. This catches sign-sensitive min/max ordering and scale normalization errors.🤖 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_minmax_index.cpp` around lines 2283 - 2387, Extend the decimal cases in CheckDecimal with a pack containing negative values and add negative thresholds to equality and range checks. Include scale-mismatched negative comparisons for checkCmp and checkIn, plus direct RoughCheck::Cmp assertions, so signed min/max ordering and decimal scale normalization are exercised while preserving existing positive and null cases.
🤖 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/tests/gtest_dm_minmax_index.cpp`:
- Around line 66-116: Rename the newly introduced snake_case variables
throughout the MinMaxIndexWriteTest fixture and its related test code to the
specified camelCase names: decimalColId, parentPath, fileProvider, dmFile,
columnData, delMark, colData, delMarkColumn, checkResults, and testCase. Update
every declaration and reference consistently, including the additional affected
test section.
- Around line 1989-2013: The WriteDecimalMinMaxIndex test must validate restored
Decimal min/max index behavior, not just non-zero sizes. After DMFile::restore,
use the existing DMFilePackFilter read path or inspect the restored MinMaxIndex
to verify a known matching decimal predicate is accepted and a known
non-matching predicate is rejected, while preserving the current size
assertions.
---
Nitpick comments:
In `@dbms/src/Storages/DeltaMerge/tests/gtest_dm_minmax_index.cpp`:
- Around line 274-287: Rename the helper isNullableDateType to isNullableType to
reflect that it handles all nullable test datatypes, including Int64, date/time,
and Decimal64. Update every call site and declaration consistently, without
changing its switch logic or return behavior.
- Around line 2283-2387: Extend the decimal cases in CheckDecimal with a pack
containing negative values and add negative thresholds to equality and range
checks. Include scale-mismatched negative comparisons for checkCmp and checkIn,
plus direct RoughCheck::Cmp assertions, so signed min/max ordering and decimal
scale normalization are exercised while preserving existing positive and null
cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 63af9f60-89bf-45cb-b8be-79398ef684c2
📒 Files selected for processing (2)
dbms/src/Storages/DeltaMerge/tests/gtest_dm_minmax_index.cppdocs/note/rough_set_filter_support_decimal.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/note/rough_set_filter_support_decimal.md
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
dbms/src/Storages/tests/gtest_filter_parser.cpp (2)
1023-1047: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover both decimal type cases and scale-aware literals.
This fixture contains one decimal column definition and whole-number literals only. Add a second case for the other supported decimal type, use fractional literals with different scales, and assert the decoded value or index request domain. The current assertions can pass even if scale propagation is incorrect.
🤖 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 1023 - 1047, Expand FilterParserTest.DecimalColumn to cover both supported decimal column types, adding a second fixture with the alternate type metadata. Replace the current whole-number predicates with fractional literals using different scales, and assert the decoded decimal values or index request domains so scale propagation is explicitly validated rather than relying only on operator names and column IDs.
1026-1045: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the new C++ identifiers to camelCase.
Rename
table_info_json,expect_rs_operator,rs_operator, andexpected_nametotableInfoJson,expectRsOperator,rsOperator, andexpectedName.As per coding guidelines, method and variable names in
**/*.{cpp,h,hpp}must usecamelCase.Proposed rename
- const String table_info_json = ... + const String tableInfoJson = ... - auto expect_rs_operator = [&](const String & query, const String & expected_name) { - auto rs_operator = generateRsOperator(table_info_json, query); + auto expectRsOperator = [&](const String & query, const String & expectedName) { + auto rsOperator = generateRsOperator(tableInfoJson, query);🤖 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 1026 - 1045, Rename the identifiers in this test to camelCase: table_info_json to tableInfoJson, expect_rs_operator to expectRsOperator, rs_operator to rsOperator, and expected_name to expectedName, updating all references consistently.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 `@dbms/src/Storages/tests/gtest_filter_parser.cpp`:
- Around line 1035-1039: In the expect_rs_operator lambda, replace the non-fatal
size check for rs_operator->getColumnIDs() with a fatal assertion so execution
stops before indexing when the collection is empty; retain the existing index
assertion afterward.
---
Nitpick comments:
In `@dbms/src/Storages/tests/gtest_filter_parser.cpp`:
- Around line 1023-1047: Expand FilterParserTest.DecimalColumn to cover both
supported decimal column types, adding a second fixture with the alternate type
metadata. Replace the current whole-number predicates with fractional literals
using different scales, and assert the decoded decimal values or index request
domains so scale propagation is explicitly validated rather than relying only on
operator names and column IDs.
- Around line 1026-1045: Rename the identifiers in this test to camelCase:
table_info_json to tableInfoJson, expect_rs_operator to expectRsOperator,
rs_operator to rsOperator, and expected_name to expectedName, updating all
references consistently.
🪄 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: 4d5cb96b-588b-4c74-a8ee-0bf415d8a2f2
📒 Files selected for processing (2)
dbms/src/Storages/DeltaMerge/FilterParser/FilterParser.cppdbms/src/Storages/tests/gtest_filter_parser.cpp
| auto expect_rs_operator = [&](const String & query, const String & expected_name) { | ||
| auto rs_operator = generateRsOperator(table_info_json, query); | ||
| EXPECT_EQ(rs_operator->name(), expected_name) << rs_operator->toDebugString(); | ||
| EXPECT_EQ(rs_operator->getColumnIDs().size(), 1); | ||
| EXPECT_EQ(rs_operator->getColumnIDs()[0], 5); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 \
'EXPECT_EQ.*getColumnIDs\(\)\.size|getColumnIDs\(\)\[0\]' \
dbms/src/Storages/tests/gtest_filter_parser.cppRepository: pingcap/tiflash
Length of output: 14927
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate GoogleTest defines to show local EXPECT_EQ/ASSERT_EQ semantics.
find . -path '*/gtest*.h' -print -quit
rg -n -C 4 '^`#define` (EXPECT_EQ|ASSERT_EQ)\(' .Repository: pingcap/tiflash
Length of output: 208
🌐 Web query:
GoogleTest EXPECT_EQ vs ASSERT_EQ semantics nonfatal assertion continues execution
💡 Result:
In GoogleTest, the difference between EXPECT_EQ and ASSERT_EQ lies in how they handle failure within the current function [1][2]. EXPECT_EQ: This is a nonfatal assertion [1][3]. If the assertion fails, it reports a nonfatal failure and allows the current function to continue executing [1][2]. This is useful when you want to report multiple failures within the same test or when the subsequent code does not depend on the result of the assertion [2][4]. ASSERT_EQ: This is a fatal assertion [1][3]. If the assertion fails, it generates a fatal failure and immediately aborts the current function [1][2]. It should be used when it does not make sense to continue the test if the condition fails—for example, if you need to dereference a pointer that must not be null [2][5]. Key Rule of Thumb: Use EXPECT_* assertions as your default choice because they allow the test to continue and reveal more potential errors [2][4]. Only use ASSERT_* when continuing execution after a failure would be inappropriate, such as when it would cause a crash (e.g., a segmentation fault) or when the remaining test logic depends on the failed condition [2][4]. Note that because ASSERT_* returns from the current function, it cannot be used in functions that return a value, as it would cause compilation issues [4]. Regardless of which macro is used, if any assertion in a test fails, the entire test case is considered to have failed [2][5].
Citations:
- 1: https://github.com/google/googletest/blob/main/docs/reference/assertions.md
- 2: https://github.com/google/googletest/blob/main/docs/primer.md
- 3: http://google.github.io/googletest/reference/assertions.html
- 4: https://stackoverflow.com/questions/2565299/using-assert-and-expect-in-googletest
- 5: http://google.github.io/googletest/primer.html
Use a fatal size assertion before indexing getColumnIDs().
EXPECT_EQ reports a failure but continues execution. If getColumnIDs().size() is 0, the following getColumnIDs()[0] dereferences outside the vector. Use ASSERT_EQ(rs_operator->getColumnIDs().size(), 1) before indexing in expect_rs_operator.
🤖 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 1035 - 1039, In
the expect_rs_operator lambda, replace the non-fatal size check for
rs_operator->getColumnIDs() with a fatal assertion so execution stops before
indexing when the collection is empty; retain the existing index assertion
afterward.
ti-chi-bot
Bot
added
needs-1-more-lgtm
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JaySon-Huang, xzhangxian1008 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 |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
|
/hold cancel |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
| case TiDB::TypeTimestamp: // For timestamp, should take time_zone into consideration while parsing `literal` | ||
| return true; | ||
| case TiDB::TypeDecimal: | ||
| case TiDB::TypeNewDecimal: |
There was a problem hiding this comment.
Non-blocking: reject statically unsupported mixed-decimal predicates during parsing.
After decimal columns are admitted here, a predicate whose literal is encoded as Int64, Uint64, or Float64 can still be parsed into a normal Equal/Greater/In operator. During roughCheck, ValueComparision then returns CanNotCompare, so every pack becomes conservatively Some. This makes the filter look enabled while providing no pruning, and it may load an index that cannot be used.
Could we detect this in parseTiCompareExpr and create Unsupported when exactly one side is decimal? Checking child.tp() == tipb::ExprType::MysqlDecimal is preferable to relying on literal.field_type(), because the encoded decimal value is the authoritative source for its scale. NULL should remain allowed, including inside IN. For example:
- bool is_timestamp_column = false;
+ bool is_timestamp_column = false;
+ bool is_decimal_column = false;
for (const auto & child : expr.children())
{
if (isColumnExpr(child))
{
- is_timestamp_column = (child.field_type().tp() == TiDB::TypeTimestamp);
+ const auto column_type = child.field_type().tp();
+ is_timestamp_column = (column_type == TiDB::TypeTimestamp);
+ is_decimal_column
+ = column_type == TiDB::TypeDecimal || column_type == TiDB::TypeNewDecimal;
break;
}
}
@@
else if (isLiteralExpr(child))
{
Field value = decodeLiteral(child);
+ const bool is_decimal_literal = child.tp() == tipb::ExprType::MysqlDecimal;
+ if (!value.isNull() && is_decimal_column != is_decimal_literal)
+ return createUnsupported(fmt::format(
+ "Mixed decimal comparison is not supported, sig={} column_decimal={} literal_type={}",
+ tipb::ScalarFuncSig_Name(expr.sig()),
+ is_decimal_column,
+ tipb::ExprType_Name(child.tp())));CanNotCompare -> Some should still remain as the runtime safety net for mismatches that are not knowable during parsing.
Source: rule/WINDTALKER-TYPE-002 | Second Opinion
f5993b2
into
pingcap:feature/release-8.5-materialized-view
3 participants
What problem does this PR solve?
Issue Number: ref #11031
Problem Summary:
Decimal columns were not supported by rough set min/max filtering. The read-side comparison needs scale-aware decimal comparison, and the write-side must persist ordinary min/max indexes for decimal columns.
What is changed and how it works?
CanNotCompare.DMFileWriter.Check List
Tests
Manual test:
Side effects
Documentation
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation