shyjsarah commented on code in PR #77:
URL:
https://github.com/apache/paimon-vector-index/pull/77#discussion_r3773745291
##########
core/src/ivfflat_io.rs:
##########
@@ -38,14 +38,31 @@ pub const IVFFLAT_HEADER_SIZE: usize = 64;
const FLAG_DELTA_IDS: u32 = 1 << 0;
const REQUIRED_FLAGS: u32 = FLAG_DELTA_IDS;
const SUPPORTED_FLAGS: u32 = REQUIRED_FLAGS;
+const IVFFLAT_WRITE_BUFFER_SIZE: usize = 64 * 1024 * 1024;
// Raw-vector scan cost scales with both rows and dimension. Below this amount,
// Rayon scheduling and list-local heap merging outweigh the saved CPU time.
const PARALLEL_FLAT_SCAN_MIN_COMPONENTS: usize = 1024 * 1024;
pub fn write_ivfflat_index(index: &IVFFlatIndex, out: &mut dyn SeekWrite) ->
io::Result<()> {
+ write_ivfflat_index_with_buffer_limit(index, out,
IVFFLAT_WRITE_BUFFER_SIZE)
+}
+
+fn write_ivfflat_index_with_buffer_limit(
+ index: &IVFFlatIndex,
+ out: &mut dyn SeekWrite,
+ buffer_limit: usize,
+) -> io::Result<()> {
let d = index.d;
let nlist = index.nlist;
validate_index_shape(index)?;
+ let bytes_per_vector = d.checked_mul(size_of::<f32>()).ok_or_else(|| {
+ io::Error::new(
+ io::ErrorKind::InvalidInput,
+ "IVF-FLAT bytes per vector overflow",
+ )
+ })?;
+ let buffer_limit = buffer_limit.max(bytes_per_vector);
Review Comment:
**Minor:** Could we keep `buffer_limit` as a hard byte limit and allow chunk
boundaries inside a vector?\n\n`buffer_limit.max(bytes_per_vector)` means a
vector wider than 64 MiB is still fully materialized in `write_buffer` and
submitted in one write. This is an improvement over the previous
implementation, but it leaves the advertised memory bound dependent on the
dimension.\n\nSince the file format is independent of write boundaries, a wide
vector could be encoded and written in multiple chunks. The wide-vector test
could then assert `max_write <= TEST_BUDGET` instead of explicitly accepting an
over-budget write.
##########
core/src/ivfflat_io.rs:
##########
@@ -38,14 +38,31 @@ pub const IVFFLAT_HEADER_SIZE: usize = 64;
const FLAG_DELTA_IDS: u32 = 1 << 0;
const REQUIRED_FLAGS: u32 = FLAG_DELTA_IDS;
const SUPPORTED_FLAGS: u32 = REQUIRED_FLAGS;
+const IVFFLAT_WRITE_BUFFER_SIZE: usize = 64 * 1024 * 1024;
// Raw-vector scan cost scales with both rows and dimension. Below this amount,
// Rayon scheduling and list-local heap merging outweigh the saved CPU time.
const PARALLEL_FLAT_SCAN_MIN_COMPONENTS: usize = 1024 * 1024;
pub fn write_ivfflat_index(index: &IVFFlatIndex, out: &mut dyn SeekWrite) ->
io::Result<()> {
+ write_ivfflat_index_with_buffer_limit(index, out,
IVFFLAT_WRITE_BUFFER_SIZE)
+}
+
+fn write_ivfflat_index_with_buffer_limit(
+ index: &IVFFlatIndex,
+ out: &mut dyn SeekWrite,
+ buffer_limit: usize,
+) -> io::Result<()> {
let d = index.d;
let nlist = index.nlist;
validate_index_shape(index)?;
+ let bytes_per_vector = d.checked_mul(size_of::<f32>()).ok_or_else(|| {
+ io::Error::new(
+ io::ErrorKind::InvalidInput,
+ "IVF-FLAT bytes per vector overflow",
+ )
+ })?;
+ let buffer_limit = buffer_limit.max(bytes_per_vector);
Review Comment:
**Minor:** Could we keep `buffer_limit` as a hard byte limit and allow chunk
boundaries inside a vector?
`buffer_limit.max(bytes_per_vector)` means a vector wider than 64 MiB is
still fully materialized in `write_buffer` and submitted in one write. This is
an improvement over the previous implementation, but it leaves the advertised
memory bound dependent on the dimension.
Since the file format is independent of write boundaries, a wide vector
could be encoded and written in multiple chunks. The wide-vector test could
then assert `max_write <= TEST_BUDGET` instead of explicitly accepting an
over-budget write.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
