Skip to content

Remove metadata from DynArray#7435

Merged
gatesn merged 2 commits intodevelopfrom
ct/remove-metadata
Apr 14, 2026
Merged

Remove metadata from DynArray#7435
gatesn merged 2 commits intodevelopfrom
ct/remove-metadata

Conversation

@connortsui20
Copy link
Copy Markdown
Contributor

@connortsui20 connortsui20 commented Apr 14, 2026

Summary

Apparently we just don't need this now? And we want to use array_serialize to get metadata instead.

This also fixes a few incorrect tests that we had.

API Changes

Removes the metadata method on DynArray.

Testing

Fixes some tests.

@connortsui20 connortsui20 added the changelog/chore A trivial change label Apr 14, 2026
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Apr 14, 2026 •
edited
Loading

Merging this PR will not alter performance

✅ 1147 untouched benchmarks
⏩ 1455 skipped benchmarks1

Comparing ct/remove-metadata (26f7426) with develop (8b587bb)

Open in CodSpeed

Footnotes

  1. 1455 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

joseph-isaacs
joseph-isaacs reviewed Apr 14, 2026
View reviewed changes
encodings/parquet-variant/src/vtable.rs
}
let concat = concat.freeze();
session.arrays().register(ParquetVariant);
session.arrays().register(Variant);
Copy link
Copy Markdown
Contributor

@joseph-isaacs joseph-isaacs Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Copy Markdown
Contributor

@gatesn gatesn Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParquetVariant is registered above.
Variant is the canonical array, it's now registrered in default ArraySession

@connortsui20
remove metadata from DynArray
c897b28 
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20 connortsui20 enabled auto-merge (squash) April 14, 2026 19:14
@connortsui20 connortsui20 disabled auto-merge April 14, 2026 19:21
@connortsui20
update file sizes
26f7426 
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
connortsui20
connortsui20 commented Apr 14, 2026
View reviewed changes
vortex-python/src/io.rs
Comment on lines 283 to +295
@@ -292,7 +292,7 @@ impl PyVortexWriteOptions {
/// ```python
/// >>> vx.io.VortexWriteOptions.compact().write(sprl, "tiny.vortex")
/// >>> os.path.getsize('tiny.vortex')
/// 55120
/// 55152
Copy link
Copy Markdown
Contributor Author

@connortsui20 connortsui20 Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gatesn is this expected with only the changes in this PR?

Copy link
Copy Markdown
Contributor

@gatesn gatesn Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol no

Copy link
Copy Markdown
Contributor Author

@connortsui20 connortsui20 Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #7435 (comment)

connortsui20
connortsui20 commented Apr 14, 2026
View reviewed changes
vortex-array/src/serde.rs
Comment on lines -724 to -725
#[test]
fn unknown_array_encoding_allow_unknown() {
Copy link
Copy Markdown
Contributor Author

@connortsui20 connortsui20 Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test didnt actually test anything, and was broken

@connortsui20 connortsui20 requested a review from gatesn April 14, 2026 19:27
@gatesn
Copy link
Copy Markdown
Contributor

gatesn commented Apr 14, 2026

Screenshot 2026-04-14 at 15 49 50

Size change is attributed to us always serializing the IDs of all encodings that exist in the registry. This PR adds dict + variant (which should have been there all along!!)

@gatesn gatesn enabled auto-merge (squash) April 14, 2026 20:04
@gatesn gatesn merged commit f747201 into develop Apr 14, 2026
169 of 181 checks passed
@gatesn gatesn deleted the ct/remove-metadata branch April 14, 2026 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joseph-isaacs joseph-isaacs joseph-isaacs left review comments

@gatesn gatesn gatesn approved these changes

Assignees

No one assigned

Labels

changelog/chore A trivial change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@connortsui20 @gatesn @joseph-isaacs