-
Notifications
You must be signed in to change notification settings - Fork 786
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
feat(5851): ArrowWriter memory usage #5967
Changes from 4 commits
b796982
b9a002f
23775bd
0702756
3f8681b
dcefe9e
678766b
1d289e6
aced4a1
3ca8839
81e5efb
27ceff8
9043f20
6485b3c
099b00e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -203,7 +203,23 @@ impl<W: Write + Send> ArrowWriter<W> { | |||||||||||||
self.writer.flushed_row_groups() | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// Returns the estimated length in bytes of the current in progress row group | ||||||||||||||
/// Returns the estimated memory usage of the current in progress row group. | ||||||||||||||
/// | ||||||||||||||
/// This includes: | ||||||||||||||
/// <already_written_encoded_byte_size> + <current_memory_size_of_unflushed_bytes> + <bytes_associated_with_processing> | ||||||||||||||
/// | ||||||||||||||
/// In the vast majority of cases our unflushed bytes are already encoded. | ||||||||||||||
pub fn memory_size(&self) -> usize { | ||||||||||||||
match &self.in_progress { | ||||||||||||||
Some(in_progress) => in_progress.writers.iter().map(|x| x.memory_size()).sum(), | ||||||||||||||
None => 0, | ||||||||||||||
Comment on lines
+216
to
+218
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It took me a while to understand that this calculation actually also includes the What do you think about making this more explicit like
Suggested change
And then change code like /// Returns the estimated total memory usage.
///
/// Unlike [`Self::get_estimated_total_bytes`] this is an estimate
/// of the current memory usage and not it's anticipated encoded size.
#[cfg(feature = "arrow")]
pub(crate) fn memory_size(&self) -> usize {
self.column_metrics.total_bytes_written as usize + self.encoder.estimated_memory_size()
} to only include the estimated memory size: /// Returns the estimated total memory buffer usage.
#[cfg(feature = "arrow")]
pub(crate) fn memory_size(&self) -> usize {
self.encoder.estimated_memory_size()
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The At first glance, it does look like we could do Per our approach to memory limiting, we should have unflushed bytes (in buffer) already be encoded. However, I believe that's not the case for the indices on the dict encoder? As such, the accounting in this PR specifically considers unflush_bytes seperately -- and pushes down the new I added a commit to help explain. Please let me know if I misunderstood. 🙏🏼 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am thinking from a user's point of view (e.g. our usecase in InfluxDB ) If I want to know how much memory the ArrowWriter is using (so I can track it, against a budget for example) what API should I use? I was worried that I couldn't use
However, after reviewing the code more closely I see the difference is that |
||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// Returns the estimated length in encoded bytes of the current in progress row group. | ||||||||||||||
/// | ||||||||||||||
/// This includes: | ||||||||||||||
/// <already_written_encoded_byte_size> + <estimated_encoded_size_of_unflushed_bytes> | ||||||||||||||
pub fn in_progress_size(&self) -> usize { | ||||||||||||||
match &self.in_progress { | ||||||||||||||
Some(in_progress) => in_progress | ||||||||||||||
|
@@ -629,7 +645,26 @@ impl ArrowColumnWriter { | |||||||||||||
Ok(ArrowColumnChunk { data, close }) | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// Returns the estimated total bytes for this column writer | ||||||||||||||
/// Returns the estimated total memory usage. | ||||||||||||||
/// | ||||||||||||||
/// Unlike [`Self::get_estimated_total_bytes`] this is an estimate | ||||||||||||||
/// of the current memory usage and not it's anticipated encoded size. | ||||||||||||||
/// | ||||||||||||||
/// This includes: | ||||||||||||||
/// <already_written_encoded_byte_size> + <current_memory_size_of_unflushed_bytes> + <bytes_associated_with_processing> | ||||||||||||||
/// | ||||||||||||||
/// In the vast majority of cases our unflushed bytes are already encoded. | ||||||||||||||
pub fn memory_size(&self) -> usize { | ||||||||||||||
match &self.writer { | ||||||||||||||
ArrowColumnWriterImpl::ByteArray(c) => c.memory_size(), | ||||||||||||||
ArrowColumnWriterImpl::Column(c) => c.memory_size(), | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// Returns the estimated total encoded bytes for this column writer. | ||||||||||||||
/// | ||||||||||||||
/// This includes: | ||||||||||||||
/// <already_written_encoded_byte_size> + <estimated_encoded_size_of_unflushed_bytes> | ||||||||||||||
pub fn get_estimated_total_bytes(&self) -> usize { | ||||||||||||||
match &self.writer { | ||||||||||||||
ArrowColumnWriterImpl::ByteArray(c) => c.get_estimated_total_bytes() as _, | ||||||||||||||
|
@@ -2894,24 +2929,28 @@ mod tests { | |||||||||||||
// starts empty | ||||||||||||||
assert_eq!(writer.in_progress_size(), 0); | ||||||||||||||
assert_eq!(writer.in_progress_rows(), 0); | ||||||||||||||
assert_eq!(writer.memory_size(), 0); | ||||||||||||||
assert_eq!(writer.bytes_written(), 4); // Initial header | ||||||||||||||
writer.write(&batch).unwrap(); | ||||||||||||||
|
||||||||||||||
// updated on write | ||||||||||||||
let initial_size = writer.in_progress_size(); | ||||||||||||||
assert!(initial_size > 0); | ||||||||||||||
assert_eq!(writer.in_progress_rows(), 5); | ||||||||||||||
let initial_memory = writer.memory_size(); | ||||||||||||||
assert!(initial_memory > 0); | ||||||||||||||
|
||||||||||||||
// updated on second write | ||||||||||||||
writer.write(&batch).unwrap(); | ||||||||||||||
assert!(writer.in_progress_size() > initial_size); | ||||||||||||||
assert_eq!(writer.in_progress_rows(), 10); | ||||||||||||||
assert!(writer.memory_size() > initial_memory); | ||||||||||||||
|
||||||||||||||
// in progress tracking is cleared, but the overall data written is updated | ||||||||||||||
let pre_flush_bytes_written = writer.bytes_written(); | ||||||||||||||
writer.flush().unwrap(); | ||||||||||||||
assert_eq!(writer.in_progress_size(), 0); | ||||||||||||||
assert_eq!(writer.in_progress_rows(), 0); | ||||||||||||||
assert_eq!(writer.memory_size(), 0); | ||||||||||||||
assert!(writer.bytes_written() > pre_flush_bytes_written); | ||||||||||||||
|
||||||||||||||
writer.close().unwrap(); | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,10 +93,19 @@ pub trait ColumnValueEncoder { | |
/// Returns true if this encoder has a dictionary page | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. likewise, while this trait is marked |
||
fn has_dictionary(&self) -> bool; | ||
|
||
/// Returns the estimated total memory usage of the encoder | ||
alamb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// | ||
/// This should include: | ||
/// <already_written_encoded_byte_size> + <current_memory_size_of_unflushed_bytes> + <bytes_associated_with_processing> | ||
fn estimated_memory_size(&self) -> usize; | ||
|
||
/// Returns an estimate of the dictionary page size in bytes, or `None` if no dictionary | ||
fn estimated_dict_page_size(&self) -> Option<usize>; | ||
|
||
/// Returns an estimate of the data page size in bytes | ||
/// | ||
/// This should include: | ||
/// <already_written_encoded_byte_size> + <estimated_encoded_size_of_unflushed_bytes> | ||
fn estimated_data_page_size(&self) -> usize; | ||
|
||
/// Flush the dictionary page for this column chunk if any. Any subsequent calls to | ||
|
@@ -227,6 +236,16 @@ impl<T: DataType> ColumnValueEncoder for ColumnValueEncoderImpl<T> { | |
self.dict_encoder.is_some() | ||
} | ||
|
||
fn estimated_memory_size(&self) -> usize { | ||
match &self.dict_encoder { | ||
// For this DictEncoder, we have unencoded bytes in the buffer. | ||
Some(encoder) => encoder.estimated_memory_size(), | ||
// Whereas for all other encoders the buffer contains encoded bytes. | ||
// Therefore, we can use the estimated_data_encoded_size. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe the phrase "all other encoders" should be changed to "it's presumed for all other encoders" since moving target. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I reviewed this carefully and I think it is worth not intermixing the encoded estimate with the memory usage, so I took the liberty of implementing |
||
_ => self.encoder.estimated_data_encoded_size(), | ||
} | ||
} | ||
|
||
fn estimated_dict_page_size(&self) -> Option<usize> { | ||
Some(self.dict_encoder.as_ref()?.dict_encoded_size()) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also need to account for the interner's dedup hash table. I added some code to do this in XX
Also, in general a more accurate estimate of memory usage is
capacity() * element_size