Add outer nullability to Union - #8798
Conversation
c5f26f5 to
7db4fac
Compare
Polar Signals Profiling ResultsLatest Run
Previous Runs (2)
Powered by Polar Signals Cloud |
Benchmarks: Vortex queriesVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.027x ➖, 0↑ 0↓)
datafusion / parquet (0.930x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.012x ➖, 0↑ 0↓)
duckdb / parquet (1.004x ➖, 0↑ 0↓)
No file size changes detected. |
Merging this PR will improve performance by 11.17%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
7db4fac to
f04a9d8
Compare
| } | ||
| _ => {} | ||
| if let (DType::Union(lhs, lhs_null), DType::Union(rhs, rhs_null)) = (self, other) { | ||
| return (lhs == rhs).then(|| DType::Union(lhs.clone(), *lhs_null | *rhs_null)); |
There was a problem hiding this comment.
so... do we do the fun thing where we try to construct a union more cleverly by looking at the types and type ids / field names?
I think there is a strong argument to do this, but also we can do this another time
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f04a9d879e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | List(_, null) | ||
| | FixedSizeList(_, _, null) | ||
| | Struct(_, null) | ||
| | Union(_, null) |
There was a problem hiding this comment.
With unions now reporting only their outer nullability here, can_coerce_from's generic eq_ignore_nullability path can accept unsafe union coercions: a target like union(value=i32) and a source like union(value=i32?) are both outer-non-nullable, so other.is_nullable() is false even though source rows may select a null child. That allows child nullability to be narrowed; special-case unions in coercion or otherwise check variant nullability recursively before returning true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is actually wrong by our new definition of null unions, as we say outer null value for union is not the same as a union of nullable things
| } | ||
| _ => {} | ||
| if let (DType::Union(lhs, lhs_null), DType::Union(rhs, rhs_null)) = (self, other) { | ||
| return (lhs == rhs).then(|| DType::Union(lhs.clone(), *lhs_null | *rhs_null)); |
There was a problem hiding this comment.
I think these rules will go away. They need a replacement but essentially this has to be part of the higher level that integrates with vortex
2 participants
Tracking issue: #8769
Adds the breaking Union dtype foundation before scalar and array support.
Changes
DType::Unionto carry independent outer nullabilityThis changes the serialized Serde representation of Union dtypes and will require the compatibility override to be reviewed explicitly.