EeshanBembi commented on PR #18137: URL: https://github.com/apache/datafusion/pull/18137#issuecomment-3419951501
> I hate to ask this upfront, but how much of this code is LLM generated? Do you have a full understanding of what it does? I find a lot of this code quite baffling and not written in a Rust-like way. > > For example in `coerce_types`, the comments are too verbose are state what is happening (a lot of the time providing no benefit as the code is straightforward enough in what it does) but there are no comments explaining _why_ choices were made. There are also odd choices like defaulting to `Int32` type if all inner list types are null. > > Not to mention the CI checks aren't passing. Thanks for the honest review, and sorry this should have been a Draft PR. I was trying out some ideas around concat and list coercion related to issue #18020 and I did use some AI help for boilerplate while experimenting, but I do understand the code and take responsibility for it. I agree the comments read like explanations of what rather than why, the Int32 fallback for all-null inner list types was a quick experiment. I will convert this to Draft now, remove the noisy and misleading comments (including the one that says it delegates to array_concat_inner), avoid duplicating coerce_types logic in return_type since inputs are already coerced, switch to ScalarFunctionArgs::number_rows instead of inferring num_rows, refactor toward idiomatic Rust, and then ask for another review once everything is cleaned up and passing. Thanks again for the direct feedback. -- 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]
