vrjdev commented on PR #58946: URL: https://github.com/apache/spark/pull/58946#issuecomment-5784386382
Thanks for the detailed review, @tdcmeehan and for explaining the math. Fractional encoding only holds up to epochMicros < 2^43 (~1970-04-12); past that a Double's mantissa can't also hold the sub-microsecond fraction, so it silently degrades exactly as you describe, and the tests I'd added happened to sit inside that safe window. I've pushed a fix taking your first option: toDouble/fromDouble's AnyTimestampNanoType case is back to the epochMicros-only conversion (matching the original commit and the same |epochMicros| <= 2^53 exactness domain as the existing TimestampType/TimestampNTZType case), and the comment now states the microsecond-resolution limit plainly instead of claiming a lossless nanosecond round-trip. EstimationUtilsSuite now has a test at a realistic 2022-magnitude epochMicros (the value from your write-up) that pins the documented collision, plus a monotonicity check across microseconds — instead of only exercising near-epoch values. JoinEstimation.computeByHistogram's switch to the shared toDouble and UnionEstimation's type-widening are unchanged, since neither depends on the fractional encoding. Agreed that real nanosecond-resolution CBO estimation isn't achievable through a Double at these magnitudes and belongs in SPARK-57839 rather than here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
