Support TiDB NullEQ pushdown#10946
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:
📝 WalkthroughWalkthroughThis PR adds TiDB NullEQ ( ChangesNullEQ Push-Down Implementation
Estimated code review effort: 4 (Complex) | ~60 minutes 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
🤖 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/Index/tests/gtest_dm_minmax_index.cpp`:
- Around line 2448-2455: The DAGQueryInfo construction in
gtest_dm_minmax_index.cpp is using the wrong overload and is missing required
parameters. Update the std::make_unique<DAGQueryInfo> call in the minmax index
test to match the full constructor signature used by ParseIn, including the
missing fts_query_info and used_indexes arguments in the correct order. Use the
DAGQueryInfo constructor and the surrounding ParseIn-style argument sequence as
the reference for locating and fixing this call.
In `@dbms/src/Storages/DeltaMerge/tests/gtest_dm_filter_parser_nulleq.cpp`:
- Around line 95-103: The DAGQueryInfo construction in the null-eq parser test
is missing required arguments, so update the std::make_unique<DAGQueryInfo> call
to pass both fts_query_info and empty_used_indexes in the correct position. Use
the existing local values and match the constructor signature in DAGQueryInfo
exactly so the test builds against the current API.
🪄 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: fcd47c8c-fa93-4f5a-8ca5-a5a90c7d977b
📒 Files selected for processing (13)
dbms/src/Flash/Coprocessor/DAGUtils.cppdbms/src/Flash/Coprocessor/tests/gtest_tidb_null_eq_func.cppdbms/src/Functions/FunctionsComparison.cppdbms/src/Functions/tests/gtest_tidb_null_eq.cppdbms/src/Storages/DeltaMerge/Filter/NullEqual.hdbms/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/tests/gtest_dm_minmax_index.cppdbms/src/Storages/DeltaMerge/tests/gtest_dm_filter_parser_nulleq.cpp
|
Hi @windtalker and maintainers, |
|
Hi @yy782 , thanks a lot for your interest in TiFlash and for working on this issue. Sorry that we did not notice earlier that this issue had been assigned to you, while another implementation was already ongoing on our side. This caused two overlapping PRs for the same issue, and I apologize for the confusion. After reviewing both PRs, I would prefer to use #10946 as the base implementation. The main reason is that #10946 introduces a dedicated That said, your PR #10954 has very valuable test coverage, especially the richer type-combination tests and the fullstack end-to-end test. I think those tests would be a good supplement to #10946. So my suggestion is to keep #10946 as the core implementation, and incorporate the additional test coverage from #10954. We will make sure your contribution is properly recognized when doing that. Does this sound reasonable to you? |
|
Thanks @JaySon-Huang! I'm happy to collaborate. Please let me know how you'd like me to help integrate the tests. |
|
Hi @yy782, Thanks for being willing to collaborate. I think a good next step would be to use #10946 as the base implementation. You can first run: gh pr checkout 10946This will check out
A few notes:
After that, you can either force-push the updated changes to your current |
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
DeltaMerge: fix rough-set semantics for NullEQ - add a dedicated `NullEqual` rough-set operator instead of lowering to `Equal` - teach minmax index to evaluate nullable `NullEQ` with null-safe semantics - keep the old minmax-index compatibility path conservative when it cannot distinguish pure-null from mixed packs - add regression coverage for nullable packs and old minmax-index compatibility - update the new NullEQ test files introduced in pingcap#10726 to use copyright year 2026
- Add NullEQ function integration test in tests/fullstack-test/expr/ - Test covers basic NullEQ usage with NULL values and non-NULL comparisons - Tests nulleq(a, Null), nulleq(b, null), nulleq(a, 1), nulleq(b, 1) scenarios Signed-off-by: yy <736262857@qq.com>
- Add unit tests for NullEQ (tidbNullEQ) function covering various data types Signed-off-by: yy <736262857@qq.com>
- Add generateNullEqualOperator helper function to create NullEqual RSOperator for various data types - Add MinMaxIndexTest.NullEqual test case to verify NullEqual RS Filter behavior Signed-off-by: yy <736262857@qq.com>
Co-authored-by: JaySon <tshent@qq.com>
Co-authored-by: JaySon <tshent@qq.com>
Co-authored-by: JaySon <tshent@qq.com>
Co-authored-by: JaySon <tshent@qq.com>
Co-authored-by: JaySon <tshent@qq.com>
windtalker
force-pushed
the
support_nulleq
branch
from
aa09227 to
d5e8b12
Compare
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
ti-chi-bot
Bot
added
needs-1-more-lgtm
| auto res_col = ColumnUInt8::create(); | ||
| auto & res_data = res_col->getData(); | ||
| res_data.assign(other_nullmap.begin(), other_nullmap.end()); | ||
| block.getByPosition(result).column = std::move(res_col); |
There was a problem hiding this comment.
| auto res_col = ColumnUInt8::create(); | |
| auto & res_data = res_col->getData(); | |
| res_data.assign(other_nullmap.begin(), other_nullmap.end()); | |
| block.getByPosition(result).column = std::move(res_col); | |
| block.getByPosition(result).column = assert_cast<const ColumnNullable &>(*other_col).getNullMapColumnPtr(); |
There was a problem hiding this comment.
This is a little risk since nullmap maybe modified without check its ref_count
Co-authored-by: Liqi Geng <gengliqiii@gmail.com>
Co-authored-by: Liqi Geng <gengliqiii@gmail.com>
| : RSResults(pack_count, RSResult::Some); | ||
| } | ||
|
|
||
| ColumnRangePtr buildSets(const google::protobuf::RepeatedPtrField<tipb::ColumnarIndexInfo> & index_infos) override |
There was a problem hiding this comment.
The inverted index will ignore null values. So maybe it's better to return UnsupportedColumnRange::create().
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gengliqi, JaySon-Huang 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:
|
4 participants
What problem does this PR solve?
Issue Number: close #5102
Problem Summary:
TiDB can push down NullEQ expressions, but TiFlash does not fully recognize and execute the pushed-down NullEQ semantics yet, including rough-set filtering in DeltaMerge.
What is changed and how it works?
This PR adds TiDB NullEQ function support in TiFlash, wires NullEQ pushdown handling, adds execution tests, and fixes DeltaMerge rough-set/min-max index semantics for NullEQ.
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
New Features
<=>) support end-to-end:tidbNullEQfunction and DeltaMergenull_equalrough filtering, including correct query parsing forNULLcases.Bug Fixes
NULL-safe equality evaluation in MinMaxIndex for nullable columns, including mixedNULL/non-NULLpacks.Stringmin/max boundary extraction used by these checks.Tests
tidbNullEQ/null_equal, including negation and collator handling.