andygrove commented on PR #5050: URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-5204367900
One suggestion: `QueryPlanSerde.exprToProtoInternal` is a single funnel that every expression passes through, and it recurses into children, so a guard there on `expr.dataType` covers everything in one place — `Cast(Decimal(10,-2) as bigint)` is caught because the child attribute's own type gets checked on recursion, and the arithmetic cases fall out the same way. I'd build it on `SupportLevel.containsType`, which already walks array/map/struct at every nesting level, so nested negative-scale decimals come for free. A small helper next to `SupportLevel.strictFloatingPointReason` (same shape, returning `Option[String]`) called from the top of `exprToProtoInternal` would do it, and it keeps per-expression granularity plus the normal `withFallbackReason` path so the reason shows up in extended explain. Nothing is needed on the scan side — the Parquet spec requires `scale >= 0`, so negative-scale decimals can only arise from expressions. The tests you've added are the valuable part here and will cover this just as well. -- 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]
