Skip to content

Improve data chunk #5082

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Improve data chunk #5082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0fe7293
Fix typo in DataChunkIterator
sferik Jul 11, 2025
f1d91c9
Replace the manual implementation of Clone for Chunk<T> with #[derive…
sferik Jul 11, 2025
7d7b0fd
Implemented Clone for MaterializedLogRecord
sferik Jul 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rust/segment/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ impl ChromaError for LogMaterializerError {
/// Instead of cloning or holding references to log records/segment data, this struct contains owned values that can be resolved to the referenced data.
/// E.x. `final_document_at_log_index: Option<usize>` is used instead of `final_document: Option<&str>` to avoid holding references to the data.
/// This allows `MaterializedLogRecord` (and types above it) to be trivially Send'able.
#[derive(Debug)]
#[derive(Debug, Clone)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I saw this was added after I hit approve. We generally don't expose Clone on types that we don't intend to clone, as a defensive measure. Was there a strong reason for this change, would you be ok to remove it?

Copy link
Author

@sferik sferik Jul 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The MaterializeLogsResult struct contains a Chunk<MaterializedLogRecord> field (on line 481).

Without this commit, tests were failing with the following error:

   Compiling chroma-segment v0.1.0 (/Users/erik/Code/Rust/chroma/rust/segment)
error[E0277]: the trait bound `MaterializedLogRecord: Clone` is not satisfied
   --> rust/segment/src/types.rs:481:5
    |
478 | #[derive(Debug, Clone)]
    |                 ----- in this derive macro expansion
...
481 |     materialized: Chunk<MaterializedLogRecord>,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Clone` is not implemented for `MaterializedLogRecord`
    |
    = note: required for `Chunk<MaterializedLogRecord>` to implement `Clone`
help: consider annotating `MaterializedLogRecord` with `#[derive(Clone)]`
    |
126 + #[derive(Clone)]
127 | struct MaterializedLogRecord {
    |

For more information about this error, try `rustc --explain E0277`.
error: could not compile `chroma-segment` (lib) due to 1 previous error
error: command `/opt/homebrew/bin/cargo test --no-run --message-format json-render-diagnostics` exited with code 101

This patch fixes that problem so the tests all pass. (Sorry for not running the tests locally before submitting my initial patch.)

struct MaterializedLogRecord {
// False if the record exists only in the log, otherwise true.
offset_id_exists_in_segment: bool,
Expand Down
19 changes: 5 additions & 14 deletions rust/types/src/data_chunk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,11 @@
use std::sync::Arc;

#[derive(Debug)]
#[derive(Debug, Clone)]
pub struct Chunk<T> {
data: Arc<[T]>,
visibility: Arc<[bool]>,
}

impl<T> Clone for Chunk<T> {
fn clone(&self) -> Self {
Chunk {
data: self.data.clone(),
visibility: self.visibility.clone(),
}
}
}

impl<T> Chunk<T> {
pub fn new(data: Arc<[T]>) -> Self {
let len = data.len();
Expand Down Expand Up @@ -82,20 +73,20 @@ impl<T> Chunk<T> {
/// The iterator returns a tuple of the element and its index
/// # Returns
/// An iterator over the visible elements in the data chunk
pub fn iter(&self) -> DataChunkIteraror<'_, T> {
DataChunkIteraror {
pub fn iter(&self) -> DataChunkIterator<'_, T> {
DataChunkIterator {
chunk: self,
index: 0,
}
}
}

pub struct DataChunkIteraror<'a, T> {
pub struct DataChunkIterator<'a, T> {
chunk: &'a Chunk<T>,
index: usize,
}

impl<'a, T> Iterator for DataChunkIteraror<'a, T> {
impl<'a, T> Iterator for DataChunkIterator<'a, T> {
type Item = (&'a T, usize);

fn next(&mut self) -> Option<Self::Item> {
Expand Down