Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch from Row to_datum_iter/packer to copying #27976

Closed
antiguru opened this issue Jul 1, 2024 · 2 comments · Fixed by #28139
Closed

Switch from Row to_datum_iter/packer to copying #27976

antiguru opened this issue Jul 1, 2024 · 2 comments · Fixed by #28139
Labels
C-refactoring Category: replacing or reorganizing code D-good first issue (internal) Difficulty: Good for newcomers joining Materialize

Comments

@antiguru
Copy link
Member

antiguru commented Jul 1, 2024

At the moment, there are many places where we do the following:

let row: &Row = ...;
let mut packer = other_row.packer();
packer.extend(row.to_datum_iter());

This takes all datums from row and pushes them into other_row by means of decoding and encoding each individual item. There is an obvious way to make this faster based on the observation that the decoding/encoding is idempotent: We can instead just copy the bytes backing the datums. This probably means to skip some ToDatumIter::to_datum_iter calls since we're lacking specialization.

@antiguru antiguru added the C-refactoring Category: replacing or reorganizing code label Jul 1, 2024
@bosconi bosconi added the D-good first issue (internal) Difficulty: Good for newcomers joining Materialize label Jul 1, 2024
@bosconi bosconi assigned bosconi and unassigned bosconi Jul 1, 2024
@antiguru
Copy link
Member Author

It might be worth going though all uses of RowPacker::extend, because there are several that append a whole row (or chaining a row with another iterator.)

@sdht0
Copy link
Contributor

sdht0 commented Jul 10, 2024

Thanks @antiguru. I opened #28139.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-refactoring Category: replacing or reorganizing code D-good first issue (internal) Difficulty: Good for newcomers joining Materialize
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants