fix(spark): report interval_year overflow instead of wrapping - #1140
Conversation
|
I started looking at this one already. Just trying to make sense of all findings Claude produced before confronting you with them. |
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Guard the flattened total rather than the intermediate product: Math.multiplyExact(expr.years(), 12) throws on year/month pairs whose total months does fit an Int, and for interval_year the spec makes only the total significant — it treats 1001y -12000m as a legal spelling of 1y 0m. (178956971, -12), (178956971, -5) and (-178956971, 12) all have totals inside Int range and converted correctly before this change; they now throw.
The first two suggestions are one edit — the second needs the first; the rest is comment and coverage polish. Filed #1235, #1236, #1237 and #1238 for siblings I hit while reading this, the first of which is the same wrap still live in isthmus.
years and months are both int32 on the wire and Spark's physical type is a months Int, so nothing widens the arithmetic the way the day-time literal is widened. Above 178,956,970 years the product wrapped, and a large positive interval came out negative — Int.MaxValue years became -12 months. Far outside the spec's 10,000-year bound, so it takes a producer that is already well past it; the point is that the two adjacent literal conversions now take the same stance on the same arithmetic.
0b45379 to
28dee5e
Compare
Math.multiplyExact on the years alone refuses a mixed-sign interval whose total still fits Spark's months Int: 178,956,971 years and -12 months is 2,147,483,640 months, inside the carrier. Only the total is significant, so widen once and check where the value actually has to fit. The 12 comes from Util now, beside the day-time path's own constants. The tests cover the boundary only the addition crosses (178,956,970 years and 8 months), Int.MinValue years, which wrapped to a zero-length interval rather than a sign flip, and the mixed-sign total that must still convert. The spec's maximum is spelled with a nonzero months component, so deleting the months term no longer passes.
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
All five taken, and the check now sits where the value actually has to fit. The mutations that survived round 1 no longer do — dropping the months term, dropping the widening, or relaxing the exactness each fails the suite now, on all three variants.
Agreed on leaving both overflow messages bare: naming the value and the range on the year-month path alone would re-split the two literals on a second axis, which is the thing this PR closes. Filed #1271 for the pair.
One non-blocking suggestion left, on the type of the new constant.
The two carrier constants beside it are Long, and with this one Long the flattened total is Long arithmetic without a .toLong at the call site. The guard was one dropped widening away from wrapping again; the type makes that edit inexpressible rather than caught.
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
LGTM, #1272 will fix the editorconfig check
2 participants
ToSparkExpression.visit(IntervalYearLiteral)flattened years and months with uncheckedIntarithmetic, and Spark's physical type forYearMonthIntervalis a single monthsInt. A large year count outran that carrier silently: 178,956,971 years came out as −2,147,483,644 months, andInt.MinValueyears as exactly 0 — a zero-length interval rather than a sign flip. It throwsArithmeticExceptionnow.Only the total is significant, so the check sits on it rather than on the intermediate product: 178,956,971 years with −12 months is 2,147,483,640 months and still converts. The bound is the carrier's, not the spec's much tighter 10,000-year one, so reaching it takes a producer already far past that.
Closes #1138.