Skip to content

planner: fix time range extraction when time column is on the right side#68826

Merged
ti-chi-bot[bot] merged 1 commit into
pingcap:masterfrom
panmuyun:myu-fix_extractTimeRange
Jun 2, 2026
Merged

planner: fix time range extraction when time column is on the right side#68826
ti-chi-bot[bot] merged 1 commit into
pingcap:masterfrom
panmuyun:myu-fix_extractTimeRange

Conversation

@panmuyun
Copy link
Copy Markdown
Contributor

@panmuyun panmuyun commented Jun 1, 2026

What problem does this PR solve?

Issue Number: close #68770

Problem Summary:

extractTimeRange could not extract correct time bounds when the target time column appeared on the right side of a comparison predicate, such as '2019-10-10 10:10:10' < update_time. In that case, the extractor reused the original comparison operator without adjusting for operand order, so the lower or upper bound was missed or interpreted in the wrong direction.

This affects memtable predicate extraction paths that reuse extractTimeRange, including cluster_log, tidb_hot_regions_history, and metrics schema time filtering.

What changed and how does it work?

This PR fixes operand-order handling in extractTimeRange.

  • Extend extractColBinaryOpConsExpr to return whether the extracted column is on the left side of the binary comparison.
  • In extractTimeRange, when the target column is on the right side, normalize the comparison direction before updating the extracted time range:
    • < becomes >
    • <= becomes >=
    • > becomes <
    • >= becomes <=
  • Keep the existing behavior unchanged for equality predicates and other helper callers that only need column/constant extraction.

This PR also adds regression tests for reversed time comparisons in existing extractor test cases:

  • information_schema.cluster_log
  • information_schema.tidb_hot_regions_history
  • metrics_schema.tidb_query_duration

Check List

Tests

  • Unit test
./tools/check/failpoint-go-test.sh pkg/planner/core/operator/logicalop/logicalop_test -run 'TestClusterLogTableExtractor|TestMetricTableExtractor|TestTiDBHotRegionsHistoryTableExtractor' -count=1
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Summary by CodeRabbit

Bug Fixes

  • Fixed time range filter handling for reversed comparison operators, ensuring both column > constant and constant < column forms produce correct results

Tests

  • Added comprehensive test coverage for reversed time comparison patterns across multiple table types

@pantheon-ai
Copy link
Copy Markdown

pantheon-ai Bot commented Jun 1, 2026

@panmuyun I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details.

⏳ This process typically takes 10-30 minutes depending on the complexity of the changes.

ℹ️ Learn more details on Pantheon AI.

@ti-chi-bot ti-chi-bot Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. contribution This PR is from a community contributor. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Jun 1, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Jun 1, 2026

Hi @panmuyun. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@ti-chi-bot ti-chi-bot Bot added the sig/planner SIG: Planner label Jun 1, 2026
@tiprow
Copy link
Copy Markdown

tiprow Bot commented Jun 1, 2026

Hi @panmuyun. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 1, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

The PR fixes time range extraction for reversed predicates. extractColBinaryOpConsExpr now returns a boolean indicating whether the column appears left of the comparison operator. Time range extraction uses this to detect constant-left forms like '2019-10-10 10:10:10' < time_col, then inverts the operator before computing bounds, ensuring correct ranges for both normal and reversed forms.

Changes

Time range extraction with reversed predicate support

Layer / File(s) Summary
Extraction function signature with column position flag
pkg/planner/core/memtable_predicate_extractor.go
extractColBinaryOpConsExpr returns (colName, datums, colOnLeft) where colOnLeft indicates column position; all error/unsupported paths return ("", nil, false).
Call site adaptations for updated extraction signature
pkg/planner/core/memtable_predicate_extractor.go
OR/EQ decomposition, main EQ case, and LIKE/REGEXP pattern extractors unpack the new 3-tuple return and discard the colOnLeft boolean while preserving existing pushdown and merge behavior.
Time range extraction with operator inversion for constant-left comparisons
pkg/planner/core/memtable_predicate_extractor.go
Time range extraction captures colOnLeft; when true, the comparison operator is inverted (GT↔LT, GE↔LE) before applying existing start/end bound logic, enabling correct ranges for both time_col > const and const < time_col forms.
Test cases for reversed time comparisons
pkg/planner/core/operator/logicalop/logicalop_test/logical_mem_table_predicate_extractor_test.go
Tests for cluster log, metric, and hot regions history table extractors added/updated to verify reversed comparisons like 'start' < time correctly extract time boundaries adjusted by ±1ms.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 When constants stand on the left with pride,
And time columns hide on the other side,
We flip the dance of less-than's grace,
Greater-than takes its swapped-around place.
Now bounds flow true for every case! 🕐✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main fix: handling time range extraction when the time column appears on the right side of a comparison.
Linked Issues check ✅ Passed The PR successfully addresses issue #68770 by detecting right-side column placements and normalizing comparison operators before extracting time bounds.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the time range extraction issue and adding corresponding test cases; no out-of-scope modifications detected.
Description check ✅ Passed The PR description fully addresses the template requirements with clear issue reference, problem summary, detailed explanation of changes, test coverage documentation, and release note section.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ti-chi-bot ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 1, 2026
@ti-chi-bot ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 1, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Jun 1, 2026

[LGTM Timeline notifier]

Timeline:

  • 2026-06-01 06:05:15.036174454 +0000 UTC m=+162416.106491864: ☑️ agreed by crazycs520.
  • 2026-06-01 06:06:55.824042898 +0000 UTC m=+162516.894360278: ☑️ agreed by tangenta.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 1, 2026

Codecov Report

❌ Patch coverage is 45.45455% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.2449%. Comparing base (99e1c67) to head (868a401).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #68826        +/-   ##
================================================
- Coverage   76.3104%   75.2449%   -1.0656%     
================================================
  Files          2041       2026        -15     
  Lines        563452     570622      +7170     
================================================
- Hits         429973     429364       -609     
- Misses       132563     140988      +8425     
+ Partials        916        270       -646     
Flag Coverage Δ
integration 41.7851% <45.4545%> (+2.0066%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 60.4610% <ø> (ø)
parser ∅ <ø> (∅)
br 49.8023% <ø> (-13.0287%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lance6716
Copy link
Copy Markdown
Contributor

/ok-to-test

@ti-chi-bot ti-chi-bot Bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Jun 1, 2026
@lance6716
Copy link
Copy Markdown
Contributor

/cc @AilinKid @winoros

@ti-chi-bot ti-chi-bot Bot requested review from AilinKid and winoros June 1, 2026 09:20
Copy link
Copy Markdown
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My deleted comment is not suitable.

@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Jun 1, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crazycs520, tangenta, winoros

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added the approved label Jun 1, 2026
@panmuyun
Copy link
Copy Markdown
Contributor Author

panmuyun commented Jun 2, 2026

/retest

1 similar comment
@panmuyun
Copy link
Copy Markdown
Contributor Author

panmuyun commented Jun 2, 2026

/retest

@ti-chi-bot ti-chi-bot Bot merged commit f0db696 into pingcap:master Jun 2, 2026
36 checks passed
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Jun 2, 2026

@panmuyun: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
idc-jenkins-ci-tidb/check_dev_2 868a401 link unknown /test check-dev2

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved contribution This PR is from a community contributor. lgtm ok-to-test Indicates a PR is ready to be tested. release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

extractTimeRange fails to extract time lower bound when column is on the right side

5 participants