[fix](function) Prevent cross-row rounding in months_between - #67935
Merged
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 16679 ms |
Contributor
TPC-DS: Total hot run time: 81773 ms |
Contributor
ClickBench: Total hot run time: 14.94 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Mryange
force-pushed
the
fix-months-between-column-view
branch
from
September 14, 2026 16:33
1e57ced to
0d667ca
Compare
Contributor
Author
|
run buildall |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 16714 ms |
Contributor
TPC-DS: Total hot run time: 81701 ms |
Contributor
ClickBench: Total hot run time: 14.69 s |
Contributor
Author
|
run cloud_p0 |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated static review of exact head 0d667cad314c7aa49852cc8a80643a914aa39169.
Request changes for two issues: the broad rewrite removes the source-level both-date-constant fast path, and it changes binary64 results for asymmetric one-constant calls that could never enter the reported bug path. Both can be avoided by retaining the existing specialized/vector paths and making the both-constant raw result immutable before applying per-row rounding.
Critical checkpoint conclusions:
- Goal/correctness: the row-local value does stop rounded state from leaking into a later unrounded row. Default null handling,
ColumnViewindexing/lifetimes, zero rows, and DATEV2/TIMESTAMP_NS dispatch are otherwise sound. - Test proof/results: the added DATEV2 case reaches the target state transition (
k0=3true thenk0=6false in the same tiny bucket-6 block) and its new rounded/raw values are correct. The unrelated changes to existing unrounded outputs expose the numerical issue called out inline. - Scope/minimality: not yet satisfactory; a narrow immutable-precompute fix avoids both accepted regressions.
- Concurrency/lifecycle/memory/error handling: no shared state, locking, lifecycle, ownership, allocation-accounting, ignored-Status, or exception-boundary issue was found.
- Configuration/compatibility/parallel paths: no config, persistence, storage/protocol, rolling-upgrade, transaction, data-write, or FE-BE variable change applies. Exact floating-result compatibility is affected as noted inline; DATEV2 and TIMESTAMP_NS otherwise follow the same safe loop.
- Observability: no new logging or metric is warranted for this scalar path.
- Performance: invariant raw-date work now executes per row instead of once per block, including two out-of-line TIMESTAMP_NS conversions.
- User focus: no additional focus was provided.
Review status: complete and converged after three rounds; all candidates were independently validated, deduplicated, accepted, or dismissed, with no unresolved suspicious point. No local build or test was run because the review contract prohibits builds. The pure-text BE header-hygiene gate passed; exact-head CI reports Compile, BE UT, and P0 successful, while the cloud P0 rerun is still pending.
| -1.90322581 0.90322581 \N \N | ||
| -12.96774194 \N -12.96774193548387 \N | ||
| -13.87096774 0.87096774 -13.87096774193548 \N | ||
| -12.96774194 \N -12.967741935483872 \N |
Contributor
There was a problem hiding this comment.
[P2] Avoid changing unaffected unrounded results
These oracle changes come from calls with only one constant date (the queries at Groovy lines 79-83), so they can never enter the buggy date_consts[0] && date_consts[1] path. The rewrite nevertheless changes the returned binary64 values: for example, the old -12.96774193548387 is the nearest double to exact -402/31, while the new -12.967741935483872 is the adjacent one-ULP value; 340/31 changes the same way. This is unrelated numerical compatibility drift, and some cases become less accurate. Please keep the asymmetric/vector calculation path unchanged and narrowly fix the both-date-const path by copying an immutable precomputed raw value before per-row rounding, then restore the unaffected expected results.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
mrhhsg
approved these changes
Sep 15, 2026
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
3 participants
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.
When both date arguments are constants and
round_offvaries by row, a rounded result was reused by later rows, causinground_off = falserows to lose precision. UseColumnViewfor consistent constant, regular, and nullable column access, and calculate an independent raw value for every row before optional rounding.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)