iSparshP commented on PR #24856: URL: https://github.com/apache/datafusion/pull/24856#issuecomment-5504252070
@ryzhyk no your fix is totaly correct, sorry for the confusion! i picked up the issue and didnt notice you already had #24790 open from a couple days before, otherwise i wouldnt have duplicated it. both patches do the same thing, we both carry current_field.metadata() across on the struct/list/map rebuilds. the only real difference is style. yours adds .with_metadata(...) onto each Field::new_* call, mine routes the rebuild through the existing field_with_new_type helper so the clone carries name, nullability and metadata in one go (same way the leaf fields are already handled). purely cosmetic, and honestly your test is nicer since it checks PARQUET_FIELD_ID_META_KEY which is the actual delta/spark field id case that breaks. yours came first and it's good, so it should be the one that lands. i'm closing this in favor of #24790. if a maintainer likes the field_with_new_type version feel free to grab that one line from here, but no need. thanks for filing it with such a clear repro! -- 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]
