johntomcat7408-cmyk commented on PR #57526: URL: https://github.com/apache/spark/pull/57526#issuecomment-5077572358
> This PR overlaps with #56910 ( on the `TimeType` portions of `EstimationUtils.toDouble/fromDouble` and `FilterEstimation`. Both PRs modify the same match arms. They will conflict regardless of merge order. Consider either: > > * Rebasing on top of #56910 once it merges (removing the TimeType portions from this PR), or > * Coordinating with @yadavay-amzn to split the scope > > ### Suggestions (2) > * `CatalogColumnStat.toExternalString`/`fromExternalString` - these new types also need serialization support for persisted stats to round-trip across sessions. #56910 handles this for TimeType; TimestampNTZ and ANSI intervals still need it. > * `UnionEstimation.isTypeSupported` - the PR excludes UnionEstimation citing #53312, but that only covers TimeType. TimestampNTZ and ANSI interval types would still hit MatchError in union estimation. > > ### Verification > * `toDouble`: `value.toString.toDouble` is safe for TimeType nanos (max 86399999999999 < 2^53) > * `fromDouble`: YearMonthIntervalType → `double.toInt` (months, correct) > * `fromDouble`: DayTimeIntervalType/TimeType → `double.toLong` (micros/nanos, correct) > * All callers (`ValueInterval.apply`, `evaluateBinaryForNumeric`, `evaluateInSet`) go through the updated `toDouble`/`fromDouble` — no additional dispatch sites missed Thanks for the careful review. I agree that #56910 touches the same TimeType match arms in EstimationUtils and FilterEstimation, so the patches will conflict. There is a later scope decision that I think we should account for. After #56910 was opened and reviewed, @MaxGekk filed SPARK-57805 from #53312 and explicitly assigned TimeType statistics collection and UnionEstimation to #53312, while EstimationUtils plus Filter/Join estimation for TimeType, TimestampNTZ, and ANSI intervals were assigned to SPARK-57805: https://github.com/apache/spark/pull/53312#issuecomment-4854129451 Could @MaxGekk and @yadavay-amzn confirm which scope split we should follow before I remove the TimeType portions here? Regarding the two suggestions: * TimestampNTZ already has CatalogColumnStat serialization/deserialization on master, with StatisticsCollectionSuite coverage from SPARK-42777. ANSI interval ypes are not currently accepted by AnalyzeColumnCommand/statExprs, so adding only CatalogColumnStat serialization here would be incomplete. I think that should be handled as a separate end-to-end statistics-collection issue. * UnionEstimation on current master already supports TimestampNTZType and AnsiIntervalType. Only TimeType is missing, and #53312 adds it. Unsupported Union types are skipped for min/max propagation rather than reaching the EstimationUtils MatchError addressed by this PR. Thanks also for verifying the conversions and call sites. -- 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]
