Skip to content

chore: AggregateFnRef [de]serialize#7070

Merged
blaginin merged 7 commits intodevelopfrom
db/serialize-aggregates
Mar 19, 2026
Merged

chore: AggregateFnRef [de]serialize#7070
blaginin merged 7 commits intodevelopfrom
db/serialize-aggregates

Conversation

@blaginin
Copy link
Member

No description provided.

Signed-off-by: blaginin <github@blaginin.me>
@blaginin blaginin self-assigned this Mar 19, 2026
@blaginin blaginin added the changelog/chore A trivial change label Mar 19, 2026
@blaginin blaginin changed the title AggregateFnRef [de]serialize chore: AggregateFnRef [de]serialize Mar 19, 2026
Signed-off-by: blaginin <github@blaginin.me>
/// Serialize this aggregate function's options to bytes.
///
/// Returns `Ok(None)` if the function is not serializable.
pub fn serialize(&self) -> VortexResult<Option<Vec<u8>>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should instead be self.options().serialize() and not have this function at all

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe df97bac so it'll be similar to expr experience?

///
/// Looks up the aggregate function plugin by ID in the session's registry
/// and delegates deserialization to it.
pub fn deserialize(id: &str, metadata: &[u8], session: &VortexSession) -> VortexResult<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know whether this should live here, or AggregateSession::deserialize(&self, id: &AggregateFnId, metadata: &[u8])

Signed-off-by: blaginin <github@blaginin.me>
@blaginin blaginin enabled auto-merge (squash) March 19, 2026 18:00
@blaginin blaginin requested a review from gatesn March 19, 2026 19:22
Co-authored-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: blaginin <github@blaginin.me>
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 19, 2026

Merging this PR will improve performance by 10.34%

⚡ 2 improved benchmarks
✅ 1014 untouched benchmarks
⏩ 1522 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation patched_take_10k_contiguous_not_patches 307.4 µs 278.7 µs +10.32%
Simulation patched_take_10k_contiguous_patches 307.2 µs 278.4 µs +10.34%

Comparing db/serialize-aggregates (ab9c381) with develop (ea106e4)

Open in CodSpeed

Footnotes

  1. 1522 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.

blaginin and others added 2 commits March 19, 2026 19:32
Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: blaginin <github@blaginin.me>
Signed-off-by: blaginin <github@blaginin.me>
Copy link
Contributor

@gatesn gatesn left a comment

Choose a reason for hiding this comment

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

Excellent, thank you!

@blaginin blaginin merged commit 43ae2dc into develop Mar 19, 2026
99 of 101 checks passed
@blaginin blaginin deleted the db/serialize-aggregates branch March 19, 2026 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/chore A trivial change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants