adriangb commented on PR #20038: URL: https://github.com/apache/datafusion/pull/20038#issuecomment-3847797343
This is a really big PR doing a lot of things. The PR description is super helpful in getting a high level view, thank you for including that. ## High level concern My main high-level concern is that we have an XY problem here: what we (the community, users) really want is for `PhysicalExprAdapter` to properly handle structs and other casts. I don't think it matters if it internally uses `CastColumnExpr` or `CastExpr`. But more broadly: we want to be able to use casts in other places (SQL for example) and have them "just work". I think this requires a single expression that is able to handle all casts. So our real goal should be to make this expression. I think `cast()` is the right expression because (1) it predates `CastColumnExpr` and (2) it's standard SQL, etc. Can we merge `CastColumnExpr` into `cast()` so that `cast()` has all of the functionality of `CastColumnExpr`? If so that solves the `PhysicalExprAdapter` case as well right? ## This PR specifically Regardless I think this PR should be split up into smaller pieces. There's a lot of valuable work here that could be merged much quicker if it was part of a more focused smaller PR. In particular can some of`Schema rewriter cleanup` be pulled out into its own PR? -- 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]
