0lai0 commented on PR #5050: URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-6234083283
Heads up @andygrove @sunchao @comphead, the merge in `7c940c626` changed this PR enough that it is worth re-reading before it goes in, since the approval predates it. #6953 landed a guard for `Decimal(neg) -> integral` on `main`, with `negativeScaleDecimalToIntegralReason` and a test covering all four integral types across LEGACY, ANSI and TRY. That overlapped with half of what this PR was doing, so I took `main`'s version rather than mine and kept only the directions it does not cover: | direction | now owned by | | --- | --- | | `Decimal(neg) -> integral` | `main`, via #6953 | | `Decimal(neg) -> Timestamp` | this PR, `negativeScaleDecimalToTimestampReason` | | `integral -> Decimal(neg)` | this PR, `negativeScaleDecimalFromIntegralReason` | `canCastFromDecimal` matches each target group once and does the scale check inside that case, which keeps the de-duplication @comphead asked for while covering both the integral group from `main` and the timestamp group from here. `main`'s own test passes unchanged against that shape, so the two are equivalent. On the test side I kept `main`'s integral test as is, since it is more thorough than what I had, and narrowed mine to the two remaining directions plus the dispatcher routing assertions that `main`'s test does not make. One consequence worth flagging: `CometCast` now carries four negative-scale reason strings, counting the pre-existing to-string one. That is squarely what #7021 is about, and this merge makes it more worth doing rather than less. Also settled by the merge: `main`'s test asserts `Unsupported` for TRY as well, which matches @andygrove's call to leave the `evalMode` scoping alone, so that question is closed. Verified on the merge result: `cargo clippy --all-targets --workspace -- -D warnings` clean, `./mvnw spotless:check` clean, `./mvnw -B test-compile -Pspark-3.5,delta -Pstrict-warnings -DskipTests` clean (that is what the earlier red run caught, an `implicit numeric widening` in a test I added), and nine suites green with 605 passing and 0 failures. -- 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]
