fix(spark): accept Substrait precisions coarser than microseconds - #1128
Conversation
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
The fix for #1125 is right in substance — a coarse precision does fit Spark's microsecond representation, and scaling the literal value is the correct reading. My concern is placement: the guard you relaxed sat in the type visitor, which serves far more than literals, while the rescale only landed in the literal visits. That turns two previously-loud paths into silently-wrong ones. Details inline.
Two things that can't be anchored to the diff:
DialectGenerator.supportedTypes()still declaresmax_precision = Some(9)forPRECISION_TIMESTAMP,PRECISION_TIMESTAMP_TZandINTERVAL_DAY, and the checked-inspark/spark_dialect.yamlagrees. This PR is what decides the answer is 0..6, so a producer reading the dialect to decide what to emit is told 9 is fine and then hits the rejection your new test asserts.DialectSuitecan't catch the divergence — it only compares the generator against the file, and both hold the same number. Feeding these fromUtil.MICROSECOND_PRECISIONkeeps them in step.- The Spark→Substrait direction still hard-pins precision 6 (
ToSubstraitType.scala:154-156,ToSubstraitLiteral.scala:38,103,105), so a coarse precision that now enters comes back out as 6. For a literal that's value-lossless since it was rescaled; for a column type the type widens while the data doesn't. Worth a deliberate decision rather than a side effect.
Unrelated to the code: per AGENTS.md ("Keep PR descriptions high-signal"), the closing paragraph of the description — "Verified on all three variants ... 62 tests each" — is worth removing, since the title and body become the squash-merge commit that semantic-release turns into CHANGELOG.md and CI already reports what passed. The rationale paragraphs above it are exactly what that guidance asks for.
Spark stores timestamps and day-time intervals as a microseconds Long, and the conversion from Substrait refused any precision but 6 outright, so an interval_day<3> or precision_timestamp<0> plan did not convert at all. A coarser precision carries fewer digits, not different ones, so it fits that representation exactly. The check was load-bearing rather than merely cautious: the literal converters pass the Substrait value through verbatim, so accepting a coarser precision without scaling would have read milliseconds as microseconds. Scale the value instead, and keep rejecting a precision finer than microseconds, which genuinely does not fit. The reverse direction is unchanged: Spark's physical value is always microseconds, so it already emits precision 6.
Relaxing the guard in the type visitor covered cast targets and column types too, where nothing rescales the values, so a plan asking for millisecond resolution kept microsecond values. The type visitor goes back to requiring exactly microseconds and the literal visits rescale, which they can do because they hold the value. Also: exact arithmetic in the rescale and in the interval literal, a range message that covers both ends, and the dialect's max_precision read from the same constant as the guard.
adefe80 to
2ef2689
Compare
|
All taken except one, with the details in the threads. Every claim in the review reproduced before I touched anything — the cast evaluating to Two notes on the pair that could not be anchored to the diff.
On the reverse direction: with the type conversions back to requiring exactly 6, the asymmetry mostly closes by itself. A coarse precision now enters only on a literal, is rescaled there, and leaves as 6, which is value-lossless; a column type at a coarse precision does not enter at all, so nothing widens. The decision that remains is the one in the first thread — whether the column case should be made to work through a normalization pass. Description trimmed, and the |
|
Re-reviewed at Two of the fixes are better than what I suggested: taking Checked locally as well as in CI, since the shared Scala source compiles against two Scala versions: the suite is green on both |
2 participants
Spark stores timestamps and day-time intervals as a microseconds
Long, andUtil.assertMicrosecondsrefused any Substrait precision but 6. So a plan carryinginterval_day<3>orprecision_timestamp<0>did not convert to Spark at all, even though a coarser precision carries fewer digits than microseconds, not different ones, and fits that representation exactly.The fix is a rescale rather than a relaxed guard, and it applies only where a value is in hand.
Util.toMicrosecondsscales a coarse value into microseconds and the three temporal literal conversions use it; the type conversions keep requiring exactly microseconds.That split is the whole point. A Spark type carries no precision of its own, so mapping
precision_timestamp<3>ontoTimestampNTZTypesays nothing about the millisecond counts it describes, and a type conversion has no value at hand to rescale — a cast to a coarser precision becomes a cast between twoTimestampNTZTypes and truncates nothing, and a column declared at one has its data read as microseconds. Both previously failed loudly on the exact-6 check and still do. What changes is that a literal, which does carry its value, converts.So this covers the literal half of #1125 and leaves the column half where it was: a plan whose column is
precision_timestamp<3>is still rejected. Making that work means rescaling data rather than mapping a type — a normalization pass over the plan — which is worth deciding on deliberately rather than acquiring as a side effect here. TheCloseskeyword is off this description for that reason.A precision finer than microseconds is still rejected everywhere: it does not fit, and rounding it would be the silent kind of loss this is avoiding.
The reverse direction needs nothing, and the asymmetry mostly closes on its own: a coarse precision now enters only on a literal, is rescaled there, and leaves as 6, which is lossless; a column type at a coarse precision does not enter at all, so nothing widens.
Two things came along that are not strictly the fix:
Math.multiplyExact, and the interval literal'sdays * 86400 * 1e6does too. Neither overflows within the spec's ranges, butLong.MAX_VALUEat precision 0 used to wrap to-1000000— one second before the epoch — rather than fail.DialectGeneratordeclaredmax_precision = 9forPRECISION_TIMESTAMP,PRECISION_TIMESTAMP_TZandINTERVAL_DAY, which was already wrong before this PR, since the guard has always required 6. It readsUtil.MICROSECOND_PRECISIONnow, andspark_dialect.yamlis regenerated.core'sSparkDialectParseTestpins that number off the copied file, so it moves with it.Part of #1125. Found by @nielspardon while reviewing #1120.