adriangb commented on PR #23169: URL: https://github.com/apache/datafusion/pull/23169#issuecomment-5489686098
@paleolimbot thank you for working on this, and thank you @timsaucer for reviewing. I would like to bring up a question that is probably better asked now, as frustrating as it may be, than after merging: are these really the semantics we want? The main alternative I see is "cast stamps the metadata of the target type", which is what @paleolimbot originally wanted in #22079 and it's what @alamb proposed in https://github.com/apache/datafusion/pull/23169#discussion_r3507474686. That's easier to reason about. _I_ was the one that originally pushed us in this direction (merging but stripping extension metadata from the source), but on reflection I don't have any concrete use case for this, I was just defending the status quo (and probably should have spent more time trying to understand the consequences before having @paleolimbot put in all of the work in this PR to work around that). If we wanted to go in this direction (the result field from the cast carries the metadata from the target) I would support that change. I've put together a series of draft PRs that reach all of the new tests this PR adds, but via the target-stamps rule instead: #24831 -> #24833 -> #24834 / #24835. Stacked, they pass this PR's version of `cast_extension_type_metadata.slt` verbatim, with the full sqllogictest suite green. The main con I see to this is that it will be a breaking change in 56. We're somewhat taking a gamble with some use case or user presenting a use case the change broke, but it's a smaller gamble than I expected: across the whole sqllogictest suite the rule change moves 7 assertions, all in `metadata.slt`, and all of them were added by me in #21390. It's also worth noting #21390 only made `try_cast` match `cast`, so half of that surface has only existed since 54. In exchange we are buying ourselves IMO an easier to reason about mental model. I'm also happy to merge this as is, but I wanted to offer the option in case we are going down a more complex path because of my input. Separately, and at no fault of this PR, I don't think we should be viewing this as the minimal back port required. Back ports are to fix bugs introduced in that release or recent releases. #24721 does qualify on that basis: the projection metadata loss is a regression found upgrading to 55, which is what I scoped #24831 to ([#24721](https://github.com/apache/datafusion/issues/24721) plus the minimum cast change needed to make the projection fix not regress). But this PR is fixing longstanding inconsistent behavior, which I don't think meets the same bar. I think there's a more narrow version of #24670 which does (see https://github.com/apache/datafusion/pull/24670#discussion_r3898881697, https://github.com/apache/datafusion/pull/24831). -- 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]
