Skip to content

[Expr][Gluten] Fix decimal ceil remainder overflow#543

Open
wangxinshuo-bolt wants to merge 1 commit intobytedance:mainfrom
wangxinshuo-bolt:fix_ceill_decimal_github
Open

[Expr][Gluten] Fix decimal ceil remainder overflow#543
wangxinshuo-bolt wants to merge 1 commit intobytedance:mainfrom
wangxinshuo-bolt:fix_ceill_decimal_github

Conversation

@wangxinshuo-bolt
Copy link
Copy Markdown
Collaborator

@wangxinshuo-bolt wangxinshuo-bolt commented May 8, 2026

What problem does this PR solve?

Issue Number: close #544

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 🚀 Performance improvement (optimization)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🔨 Refactoring (no logic changes)
  • 🔧 Build/CI or Infrastructure changes
  • 📝 Documentation only

Description

Store the remainder using the divisor type B, which is wide enough for the decimal scale involved in the operation:

  B remainder = unsignedDividendRescaled % unsignedDivisor;

This keeps the rounding decision based on the full decimal-scale remainder while preserving the original result type for the final quotient.

Added regression coverage for the long-decimal ceil case and moved/refactored related decimal vector function tests so the Spark decimal test target includes the relevant coverage.

Performance Impact

  • No Impact: This change does not affect the critical path (e.g., build system, doc, error handling).

  • Positive Impact: I have run benchmarks.

    Click to view Benchmark Results
    Paste your google-benchmark or TPC-H results here.
    Before: 10.5s
    After:   8.2s  (+20%)
    
  • Negative Impact: Explained below (e.g., trade-off for correctness).

Checklist (For Author)

  • I have added/updated unit tests (ctest).
  • I have verified the code with local build (Release/Debug).
  • I have run clang-format / linters.
  • (Optional) I have run Sanitizers (ASAN/TSAN) locally for complex C++ changes.
  • No need to test or manual test.

Breaking Changes

  • No

  • Yes (Description: ...)

    Click to view Breaking Changes
    Breaking Changes:
    - Description of the breaking change.
    - Possible solutions or workarounds.
    - Any other relevant information.
    

Ported from b984a87e3c0686d1805cf1d583e35db5104ebc13 on fix_ceil_decimal.
@wangxinshuo-bolt wangxinshuo-bolt changed the title Fix decimal ceil remainder overflow [Expr][Gluten] Fix decimal ceil remainder overflow May 8, 2026
@wangxinshuo-bolt wangxinshuo-bolt marked this pull request as ready for review May 8, 2026 09:11
@wangxinshuo-bolt wangxinshuo-bolt enabled auto-merge May 8, 2026 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] ceil(long decimal) returns incorrect result

1 participant