Add UnionValue and UnionScalar#8791
Conversation
547f89b to
a05b340
Compare
7820b66 to
d9583d3
Compare
a05b340 to
bb2292d
Compare
d9583d3 to
2c72940
Compare
bb2292d to
6c0a2ea
Compare
2c72940 to
378b981
Compare
056c755 to
877e544
Compare
378b981 to
2eb8864
Compare
UnionValue and UnionScalar
877e544 to
af088ad
Compare
Merging this PR will degrade performance by 11.1%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
Polar Signals Profiling ResultsLatest Run
Previous Runs (9)
Powered by Polar Signals Cloud |
Benchmarks: Vortex queries 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.846x ✅, 2↑ 0↓)
datafusion / parquet (0.833x ✅, 2↑ 0↓)
duckdb / vortex-file-compressed (0.991x ➖, 0↑ 0↓)
duckdb / parquet (0.957x ➖, 0↑ 0↓)
No file size changes detected. |
af088ad to
3a22a55
Compare
c5f26f5 to
7db4fac
Compare
3a22a55 to
50d65b5
Compare
7db4fac to
f04a9d8
Compare
5b5d8c0 to
39be7e2
Compare
d5458d2 to
abefbe3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abefbe3917
ℹ️ 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".
e992375 to
42124e3
Compare
df88950 to
c812c67
Compare
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
c812c67 to
dd92c91
Compare
Tracking issue: #8769
Adds scalar implementation for
Uniontype in Vortex.Changes
Adds
UnionValuewhich exists inside ofScalarValueand the typed viewUnionScalar<'a>.I was debating if we should instead store aOption<Box<ScalarValue>>instead of justBox<Scalar>(which carries an extra dtype and an extra allocation even if the value is null). I think it is nice to have the dtype in there for implementation ease. Not sure if it's worth it.We use
Option<Box<ScalarValue>>instead ofBox<Scalar>so that we do not storeDTypein multiple places. If we did so, we run into synchronization issues with casting.For casting, this only allows identity and null casting (like everything else), otherwise it just panics with unimplemented. We probably want to figure this out later as it is quite valuable to cast union types.
Finally, the "default value" and "zero value" semantics here are as such:
default_value(nullable Union)-> returns an outer null.default_value(non-nullable Union)-> panics because there is no designated default variant.zero_value(Union)-> panics because a Union has no canonical zero variant.is_zero()on a null Union -> returnsNone.is_zero()on a valid Union -> returnsSome(false), even when its child is numerically zero.Note that none of these behaviors depend on variant ordering. And also I do think that it is possible that we just choose the first variant of a union to be the default, I think this is kind of opinionated, and it also doesn't solve the issue where there are 0 variants (empty union / void type). So I would prefer if we do this in a separate change.