Decimal division and AVG round half away from zero (Snowflake semantics)#43
Merged
Merged
Conversation
Snowflake rounds the last digit of decimal division and AVG results half away from zero (verified live: 5/3 = 1.666667, -5/3 = -1.666667, 5/2000000 = 0.000003 — not half-even, not truncation). Arrow's div kernel and DecimalAverager::avg both truncate toward zero, which produced the persistent <=1 ULP parity gap on TPC-H Q1/Q8 and ClickBench Q28. - DecimalAverager::avg: correct the truncated quotient with the remainder. - BinaryExpr Divide on decimals: dispatch to a fork-local kernel that replicates arrow-arith's decimal Op::Div exactly (result precision/scale derivation, overflow and divide-by-zero errors) but rounds half away from zero. Forking arrow itself was rejected as far heavier. - decimal.slt expectations regenerated; pre-existing failures in subquery/dates/encrypted_parquet/pivot slt files are unchanged (verified identical on the clean base). - Bump the workspace sqlparser pin to the Snowflake string-escape fix rev (074bd21) so downstream consumers resolve a single sqlparser instance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Snowflake string-escape fix was squash-merged into embucket-sync-df53.0.0-parser0.61.0 as 01e9e5e (content-identical to the previously pinned 074bd21 feature-branch rev). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… column, stale EXPLAIN plans - to_date: Float64 sat in the direct-cast arm, but arrow has no Float64 -> Date32 cast; route it through the Int64 two-step like Float16/Float32/Decimal (dates.slt:216 now passes, truncation semantics identical to the other float widths). - PIVOT: the value column was rendered with Expr::to_string, which keeps quote characters, so a quoted pivot column (FOR "2023_Q1" IN ...) never matched its schema field and was not consumed by the pivot. Normalize the identifier like the rest of the planner (pivot.slt nested-PIVOT case now passes). - subquery.slt: regenerate the two EXPLAIN expectations — the plans changed upstream (CoalesceBatchesExec deprecated in apache#19622, LeftMark -> RightMark swap added in apache#17651) and were never regenerated during the df53 sync; physical-plan shape only, logical plans unchanged. encrypted_parquet.slt still fails locally only (expects a no-keys read to error; environment-dependent, fails identically on the clean base, passes in CI). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Snowflake rounds the last digit of decimal division and AVG results half away from zero (verified live: 5/3 = 1.666667, -5/3 = -1.666667, 5/2000000 = 0.000003 — not half-even, not truncation). Arrow's div kernel and DecimalAverager::avg both truncate toward zero, which produced the persistent <=1 ULP parity gap on TPC-H Q1/Q8 and ClickBench Q28.
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?