Skip to content

feat: fix windows decimal casting frame#22174

Merged
comphead merged 2 commits into
apache:mainfrom
comphead:win_decimal_frame
May 14, 2026
Merged

feat: fix windows decimal casting frame#22174
comphead merged 2 commits into
apache:mainfrom
comphead:win_decimal_frame

Conversation

@comphead
Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

RANGE window frames with a DECIMAL ORDER BY column crash at runtime:

SELECT COUNT(*) OVER (
  PARTITION BY 1
  ORDER BY cast(1 as decimal(10, 0)) DESC
  RANGE BETWEEN CURRENT ROW AND 1 FOLLOWING
) FROM (SELECT 1);
-- Internal error: Uncomparable values: Decimal128(Some(0),11,0), Decimal128(Some(1),10,0).

Frame-bound arithmetic (value ± delta) widens the decimal result precision by 1 (Decimal(10,0) ± Decimal(10,0) → Decimal(11,0)). search_in_slice then compares the widened target against the
original ORDER BY column, and ScalarValue::partial_cmp rejects decimals whose precision differs — even when the scale matches and the underlying integer representation is directly comparable.

That precision-equality gate is also inconsistent with SQL semantics: DEC(10,0) 1 and DEC(20,0) 1 represent the same number and should compare equal.

What changes are included in this PR?

datafusion/common/src/scalar/mod.rs

  • ScalarValue::partial_cmp for Decimal32 / Decimal64 / Decimal128 / Decimal256: compare underlying values whenever scales match, regardless of declared precision. Different scales still
    return None (rescaling would be required).

datafusion/sqllogictest/test_files/window.slt

  • Regression block covering the reporter query verbatim plus ASC/DESC × PRECEDING/FOLLOWING, symmetric N PRECEDING AND N FOLLOWING, and a non-zero-scale DECIMAL(10,2) case over multi-row
    partitions.

@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) common Related to common crate labels May 14, 2026
Copy link
Copy Markdown
Contributor

@nuno-faria nuno-faria left a comment

Choose a reason for hiding this comment

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

Thanks @comphead, LGTM.

Comment on lines +595 to 604
(Decimal32(v1, _, s1), Decimal32(v2, _, s2)) => {
if s1.eq(s2) {
// Same scale means the underlying integer values share
// a common interpretation regardless of declared
// precision (arithmetic such as `add_checked` widens
// precision by 1 but does not change the numeric
// meaning).
v1.partial_cmp(v2)
} else {
// Two decimal values can be compared if they have the same precision and scale.
None
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be desirable to also compare decimals of different scales.

Copy link
Copy Markdown
Contributor Author

@comphead comphead May 14, 2026

Choose a reason for hiding this comment

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

Thanks @nuno-faria would you mind to clarify. if I understood the comments correctly I jotted a decimal compare query with diff scales and it worked

> SELECT cast(1.50 as decimal(10, 2)) > cast(1 as decimal(10, 0));
+-------------------------+
| Float64(1.5) > Int64(1) |
+-------------------------+
| true                    |
+-------------------------+
1 row(s) fetched. 
Elapsed 0.017 seconds.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah I guess in practice different decimals will always be cast to a common type, so this won't be an issue.

# column and failed with "Internal error: Uncomparable values".
############################################################################

# Original reporter query.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: reported?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed the query title, as it confusing, who's reporter etc

@comphead
Copy link
Copy Markdown
Contributor Author

this thing is super annoying

Run cargo test --profile ci -p datafusion-ffi --lib --tests --features integration-tests
error: error: unexpected argument 'test' found

@comphead comphead added this pull request to the merge queue May 14, 2026
Merged via the queue into apache:main with commit b426232 May 14, 2026
79 of 81 checks passed
@comphead comphead deleted the win_decimal_frame branch May 14, 2026 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to common crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows errors out when RANGE frame is decimal

2 participants