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

Row changes #1787

Merged
merged 3 commits into from Feb 3, 2020
Merged

Row changes #1787

merged 3 commits into from Feb 3, 2020

Conversation

@frankmcsherry
Copy link
Member

frankmcsherry commented Feb 2, 2020

Minor changes to Row intended to uncontroversially improve things.

/// Number of bytes required by the datum.
///
/// This is used to optimistically pre-allocate buffers for packing rows.
pub fn datum_size(datum: &Datum) -> usize {

This comment has been minimized.

Copy link
@frankmcsherry

frankmcsherry Feb 3, 2020

Author Member

I'm not very happy with where this is at the moment. Because the row mod is private it can't be repr::row::datum_size(), and repr::datum_size() doesn't communicate what it is about. It could be a Row::datum_size() method?

This comment has been minimized.

Copy link
@jamii

jamii Feb 3, 2020

Member

We could just make row public. As long as only the safe fns are public, should still be fine.

This comment has been minimized.

Copy link
@jamii

jamii Feb 3, 2020

Member

Could also be Datum::packed_size

This comment has been minimized.

Copy link
@frankmcsherry

frankmcsherry Feb 3, 2020

Author Member

Going to punt on this for the moment. @benesch indicates we might de-flatten repr at which point making it be a method in row would be great!

@frankmcsherry frankmcsherry changed the title WIP: Row changes Row changes Feb 3, 2020
@frankmcsherry frankmcsherry requested review from benesch and jamii Feb 3, 2020
src/repr/row.rs Outdated Show resolved Hide resolved
src/repr/row.rs Outdated Show resolved Hide resolved
pub struct Row {
data: Box<[u8]>,
}

/// These implementations order first by length, and then by slice contents.
/// This allows many comparisons to complete without dereferencing memory.

This comment has been minimized.

Copy link
@jamii

jamii Feb 3, 2020

Member

This is devious. I like it.

This comment has been minimized.

Copy link
@jamii

jamii Feb 3, 2020

Member

We should do it for Eq/PartialEq too.

This comment has been minimized.

Copy link
@frankmcsherry

frankmcsherry Feb 3, 2020

Author Member

I hoped they were, but maybe they aren't. As in, I hoped Rust would be bright enough to check length equality first and then go for the contents. I'll look into it.

@jamii
jamii approved these changes Feb 3, 2020
Copy link
Member

jamii left a comment

Left a lot of little comments but the overall thrust is indeed uncontroversial :)

@frankmcsherry frankmcsherry merged commit 7644948 into MaterializeInc:master Feb 3, 2020
11 checks passed
11 checks passed
buildkite/tests Build #4809 passed (14 minutes, 27 seconds)
Details
buildkite/tests/bath-lint-and-rustfmt Passed (48 seconds)
Details
buildkite/tests/bulb-bulb-full-sql-logic-tests Passed (0 seconds)
Details
buildkite/tests/bulb-short-sql-logic-tests Passed (1 minute, 16 seconds)
Details
buildkite/tests/cargo-test Passed (56 seconds)
Details
buildkite/tests/docker-build Passed (9 minutes, 38 seconds)
Details
buildkite/tests/face-with-monocle-miri-test Passed (57 seconds)
Details
buildkite/tests/paperclip-clippy-and-doctests Passed (2 minutes, 27 seconds)
Details
buildkite/tests/pipeline Passed (26 seconds)
Details
buildkite/tests/racing-car-testdrive Passed (3 minutes, 22 seconds)
Details
netlify/materializeinc/deploy-preview Deploy preview canceled.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.