Skip to content

*: fix fix pushdown cast to datetime from float for tikv#67907

Open
xhebox wants to merge 1 commit intopingcap:masterfrom
xhebox:investigate/issue-44135
Open

*: fix fix pushdown cast to datetime from float for tikv#67907
xhebox wants to merge 1 commit intopingcap:masterfrom
xhebox:investigate/issue-44135

Conversation

@xhebox
Copy link
Copy Markdown
Contributor

@xhebox xhebox commented Apr 20, 2026

What problem does this PR solve?

Issue Number: close #44135

Problem Summary: add regression test

Check List

Tests

  • Unit test
  • 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

  • Tests
    • Added regression tests to ensure consistent query results when casting float columns to datetime format, covering both generated and non-generated column scenarios.

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 20, 2026
@pantheon-ai
Copy link
Copy Markdown

pantheon-ai bot commented Apr 20, 2026

@xhebox 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 the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 20, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot bot commented Apr 20, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kennytm for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found 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

@tiprow
Copy link
Copy Markdown

tiprow bot commented Apr 20, 2026

Hi @xhebox. 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.

@xhebox
Copy link
Copy Markdown
Contributor Author

xhebox commented Apr 20, 2026

/ok-to-test

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 20, 2026

📝 Walkthrough

Walkthrough

This pull request adds two test cases to verify consistent behavior when applying CAST(<float_col> AS DATETIME) in WHERE clauses across generated and non-generated columns. Both tests create tables with identical float values and assert that queries return matching row counts regardless of column type.

Changes

Cohort / File(s) Summary
Issue #44135 Tests
pkg/executor/test/simpletest/simple_test.go, tests/realtikvtest/pushdowntest/cast_test.go
Added test functions TestIssue44135 and TestIssue44135WithTiKV to validate consistent query results when casting float values to DATETIME in generated vs. base columns. Tests create two table scenarios with identical float values and verify row count consistency using require.Equal.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A float cast to time creates quite the sight,
Generated or base, the results must align just right.
Two tables, same value, in rows they must agree,
Consistency blooms now, as it ought to be! 🌟

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% 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 mentions 'fix fix pushdown cast to datetime from float for tikv' which relates to the main objective of fixing TiKV pushdown behavior for CAST from FLOAT to DATETIME as described in issue #44135.
Description check ✅ Passed The PR description follows the template structure with issue number (close #44135), problem summary (add regression test), and completed test checklist selections.
Linked Issues check ✅ Passed The PR adds two regression tests that validate the fix for issue #44135: TestIssue44135 compares row counts for queries casting float to DATETIME on both generated and base columns, and TestIssue44135WithTiKV does the same with TiKV integration.
Out of Scope Changes check ✅ Passed All changes are regression tests directly addressing issue #44135 without unrelated modifications to other parts of the codebase.

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

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

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.11.4)

Command failed


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 ok-to-test Indicates a PR is ready to be tested. label Apr 20, 2026
@xhebox xhebox changed the title *: fix fix pushdown cast to datetime from float for tikv | tikv=pr/19532 *: fix fix pushdown cast to datetime from float for tikv Apr 20, 2026
@xhebox
Copy link
Copy Markdown
Contributor Author

xhebox commented Apr 20, 2026

/retest

Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox xhebox force-pushed the investigate/issue-44135 branch from c2dc909 to 1dfe124 Compare April 20, 2026 05:59
@tiprow
Copy link
Copy Markdown

tiprow bot commented Apr 20, 2026

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

Test name Commit Details Required Rerun command
fast_test_tiprow 1dfe124 link true /test fast_test_tiprow
tidb_parser_test 1dfe124 link true /test tidb_parser_test

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/executor/test/simpletest/simple_test.go`:
- Around line 834-845: The test currently only asserts rows1 and rows2 have
equal lengths which can miss the failure mode where both are empty; update the
assertion to pin the expected result count (e.g., expect 1 row) for both result1
and result2 by checking len(rows1) == 1 and len(rows2) == 1 (or assert both
equal to the known expected count) after executing tk.MustQuery on the
generated-column table (result1) and base-column table (result2); reference the
variables result1, result2, rows1, rows2 and the tk.MustQuery/tk.MustExec setup
so the test fails when both cases return zero rows.

In `@tests/realtikvtest/pushdowntest/cast_test.go`:
- Around line 17-23: Update the test in
tests/realtikvtest/pushdowntest/cast_test.go to assert concrete row counts for
both query variants instead of allowing zero-row equivalence: use testkit to run
the two CAST(float AS DATETIME) queries and require.Equal the expected counts
(explicit integers) for each case; additionally run EXPLAIN for the query that
should push the predicate and assert the plan string contains the coprocessor
predicate (e.g., check EXPLAIN output includes the pushed predicate text or
"cop[t]rocessor" filter fragment) to ensure the predicate was pushed to TiKV;
use the existing test utilities (testkit and require) and adjust both related
test locations (the current block and the block around lines 37-48) accordingly.
🪄 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: 192a7fd2-9fc6-4b2c-904e-56bf2fe62ce2

📥 Commits

Reviewing files that changed from the base of the PR and between b6200ce and 1dfe124.

📒 Files selected for processing (2)
  • pkg/executor/test/simpletest/simple_test.go
  • tests/realtikvtest/pushdowntest/cast_test.go

Comment thread pkg/executor/test/simpletest/simple_test.go
Comment thread tests/realtikvtest/pushdowntest/cast_test.go
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.5404%. Comparing base (0a7bdc9) to head (1dfe124).
⚠️ Report is 171 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #67907        +/-   ##
================================================
- Coverage   77.7154%   77.5404%   -0.1750%     
================================================
  Files          2016       1966        -50     
  Lines        552643     568218     +15575     
================================================
+ Hits         429489     440599     +11110     
- Misses       121412     127537      +6125     
+ Partials       1742         82      -1660     
Flag Coverage Δ
integration 40.8961% <ø> (-7.2341%) ⬇️
unit 76.6572% <ø> (+0.4265%) ⬆️

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

Components Coverage Δ
dumpling 61.5065% <ø> (+4.2943%) ⬆️
parser ∅ <ø> (∅)
br 49.3974% <ø> (-11.4566%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot bot commented Apr 20, 2026

@xhebox: The following tests 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 1dfe124 link true /test check-dev2
pull-integration-realcluster-test-next-gen 1dfe124 link true /test pull-integration-realcluster-test-next-gen

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

do-not-merge/needs-triage-completed ok-to-test Indicates a PR is ready to be tested. release-note-none Denotes a PR that doesn't merit a release note. 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.

Inconsistent query results for the same value in a GENERATED column

1 participant