fix: propagate field metadata through coalesce, nvl, and nvl2 - #24865
fix: propagate field metadata through coalesce, nvl, and nvl2#24865james-willis wants to merge 2 commits into
Conversation
|
In this PR, if the branches of the coalesce, etc don't agree on the metadata, we drop extension metadata defensively. My opinion is that this should actually be a planning time error unless all rows will hit only one branch but wanted to take the more conservative approach for now. I want to solicit opinions from the reviewers on what the desried behavior is here. |
These functions built their return Field from argument data types only, dropping field metadata (e.g. Arrow extension types) from the planned schema. Attach the metadata shared by every value-contributing argument instead. Part of apache#24860.
82761e8 to
1ea88c0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24865 +/- ##
========================================
Coverage 81.61% 81.62%
========================================
Files 1123 1123
Lines 409392 409506 +114
Branches 409392 409506 +114
========================================
+ Hits 334143 334245 +102
- Misses 55630 55637 +7
- Partials 19619 19624 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
paleolimbot
left a comment
There was a problem hiding this comment.
Thank you for this fix! I'm not a committer so I can't help with the CI but I'll review from a metadata perspective.
I think the family of functions that are affected here are ones that have >1 "compatible" (castable to a common type) inputs. While there are some extension aware type systems that can handle this (notably: vctrs via vec_ptype2() and vec_cast()), datafusion has no mechanism for that and so probably the best we can do for now is to propagate agressively and rely on engines to write analyzer or optimizer rules to enforce other behaviour.
One other place I've seen this pattern is combining values in a values literal clause. I would personally love if the logic were the same there but that might insert too much scope here.
datafusion/datafusion/expr/src/logical_plan/builder.rs
Lines 309 to 320 in d02bd78
I am curious what @timsaucer would expect to happen to metadata in CASE WHEN since that is a metadata usage I'm less familiar with!
| /// Returns the field metadata shared by every argument that can contribute a | ||
| /// value to a conditional function's result. | ||
| /// | ||
| /// Fields with a `Null` data type (untyped NULL literals) carry no metadata | ||
| /// and are ignored. If the remaining fields disagree on metadata, the result | ||
| /// carries none: propagating one argument's metadata (e.g. an Arrow extension | ||
| /// type name) would claim a type identity for values that other arguments may | ||
| /// supply without it. | ||
| pub(crate) fn unanimous_metadata<'a>( |
There was a problem hiding this comment.
I think this may be too strict for some practical things that can happen here. Notably, extension types that use JSON for parameters can have non-equal JSON bytes, and unrelated field metadata that arrived unbeknownst to a user (e.g., was present in an Arrow file or embedded Arrow schema in a Parquet file) might not agree and could break existing (and completely valid) queries.
This should probably be a very lenient combination (possibly just merging all metadata keys or returning the first encountered metadata)...engines can insert extra constraints here if they would like to but if the return field drops the extension name, extension aware functions will fail (like ours did!).
As an example, we would probably write an optimizer rule that checks for equal SedonaTypes for a case when or ifelse; however, if the return type of the case when doesn't carry extension metadata, it will fail before the optimizer rule can run.
This may also be more appropriate next to some related utilities:
datafusion/datafusion/common/src/metadata.rs
Lines 75 to 86 in d02bd78
There was a problem hiding this comment.
Thanks Dewey! This is good to consider. I will allow some more time for any other folks to chime in if they would like.
Which issue does this PR close?
CASEexpressions drop field metadata, silently stripping Arrow extension types and breaking metadata-aware UDFs #24860.Rationale for this change
COALESCE,NVL/IFNULL, andNVL2drop the field metadata of their arguments when computing their return field:return_field_from_argsbuilds the outputFieldfrom the argument data types only. For columns carrying Arrow extension types (ARROW:extension:name/ARROW:extension:metadata— e.g.arrow.uuid,geoarrow.wkb), the planned schema of any expression containing these functions silently loses the extension-type identity, and metadata-aware UDFs that dispatch on their argumentFields fail to plan over them (see #24860 for a full reproducer).This is one of several independent metadata drop sites listed in #24860. It is the plan-time schema fix for the conditional functions; it does not by itself change execution-time behavior, because these functions simplify into
CASE, which has its own drop sites (also #24860) that I intend to address in follow-up PRs.What changes are included in this PR?
unanimous_metadatahelper indatafusion/functions/src/utils.rs: returns the metadata shared by every argument field that can contribute a value to the result. Fields with aNulldata type (untyped NULL literals) are ignored; if the remaining fields disagree, the result carries no metadata rather than claiming a type identity that only some inputs have.coalesce:return_field_from_argsnow attaches the unanimous metadata of its arguments. Return type and nullability computation are unchanged.NVL/IFNULLdelegate tocoalesceand are fixed by the same change.nvl2: same, over the second and third arguments only — the first argument is only tested for NULL and never contributes a value, so its metadata is excluded.What is the testing strategy for this PR?
Unit tests in
coalesce.rsandnvl2.rsdirectly exercisereturn_field_from_args: metadata propagated when the value arguments agree, dropped when they disagree, untyped NULL arguments not blocking propagation, NVL2's test argument excluded, and nullability unchanged.There is deliberately no sqllogictest:
arrow_metadata()reads its argument field at execution time, and at execution these functions have been simplified intoCASE, whose own metadata drops (#24860) still lose the metadata en route. End-to-end SLT coverage lands with theCASEfix. (This ordering is safe: the optimizer's schema invariant intentionally ignores metadata —assert_expected_schema/logically_equivalent_names_and_types— so a plan whosecoalescecarries metadata that the simplifiedCASEdoes not does not error; it just degrades to today's behavior until theCASEfix lands.)Are there any user-facing changes?
The planned schema (e.g.
DataFrame::schema()) of expressions containingCOALESCE/NVL/IFNULL/NVL2over metadata-bearing columns now preserves that metadata. No API changes.cc @paleolimbot — this is the first slice of #24860, in the same family as your #22112; you'd mentioned on #21984 you could help shepherd this class of metadata fixes. This is my first DataFusion PR, so CI will need a committer to trigger it.