Skip to content

Commit 2cc6c68

Browse files
committed
refactor(ftindex): address review feedback on the reader foundation
- Re-export the core I/O types the public API exposes (SeekRead, ReadRequest, SliceReader) plus BytesReader/FullTextArchiveReader/FullTextHits from paimon::ftindex, so consumers can implement SeekRead or name the reader without depending on paimon-ftindex-core directly. Add a direct from_seek_read unit test. - search_with_include now borrows &RoaringTreemap (it only serializes the bitmap) and short-circuits to empty hits when the allow-list is empty, sparing callers a clone and a wasted query. - Add a Bytes-backed BytesReader and use it in from_input_file so a whole-file read is not copied a second time via SliceReader's Vec<u8>; correct the peak-memory doc accordingly.
1 parent 1ad1680 commit 2cc6c68

2 files changed

Lines changed: 84 additions & 13 deletions

File tree

crates/paimon/src/ftindex/mod.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,3 +20,10 @@
2020
//! archive format.
2121
2222
pub mod reader;
23+
24+
// Re-export the core I/O types that appear in this module's public API so that
25+
// consumers can implement `SeekRead` (e.g. a remote range-reader) or name the
26+
// in-memory reader while depending only on `paimon`, without pulling in
27+
// `paimon-ftindex-core` directly.
28+
pub use paimon_ftindex_core::io::{ReadRequest, SeekRead, SliceReader};
29+
pub use reader::{BytesReader, FullTextArchiveReader, FullTextHits};

crates/paimon/src/ftindex/reader.rs

Lines changed: 77 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,10 @@
1717

1818
//! Reader over the `paimon-ftindex-core` v1 archive format.
1919
20-
use paimon_ftindex_core::io::{SeekRead, SliceReader};
20+
use std::io;
21+
22+
use bytes::Bytes;
23+
use paimon_ftindex_core::io::{ReadRequest, SeekRead};
2124
use paimon_ftindex_core::{FullTextIndexReader, FullTextSearchResult};
2225
use roaring::RoaringTreemap;
2326

@@ -44,7 +47,7 @@ impl From<FullTextSearchResult> for FullTextHits {
4447
/// `paimon-ftindex-core` engine (v1 archive format).
4548
///
4649
/// Generic over any `SeekRead` implementation, allowing both whole-file
47-
/// (via `SliceReader`) and streaming/range-read strategies.
50+
/// (via `BytesReader`/`SliceReader`) and streaming/range-read strategies.
4851
pub struct FullTextArchiveReader<R: SeekRead + 'static> {
4952
inner: FullTextIndexReader<R>,
5053
}
@@ -70,13 +73,22 @@ impl<R: SeekRead + 'static> FullTextArchiveReader<R> {
7073
}
7174

7275
/// Like [`search`](Self::search) but restricts results to `include_row_ids`
73-
/// (the live-row allow-list).
76+
/// (the live-row allow-list). The bitmap is only serialized, never retained,
77+
/// so it is borrowed to spare callers a clone when reusing an allow-list
78+
/// across archives. An empty allow-list admits no rows, so this returns
79+
/// empty hits immediately without running the query.
7480
pub fn search_with_include(
7581
&self,
7682
query_json: &str,
7783
limit: usize,
78-
include_row_ids: RoaringTreemap,
84+
include_row_ids: &RoaringTreemap,
7985
) -> crate::Result<FullTextHits> {
86+
if include_row_ids.is_empty() {
87+
return Ok(FullTextHits {
88+
row_ids: Vec::new(),
89+
scores: Vec::new(),
90+
});
91+
}
8092
let mut filter_bytes = Vec::with_capacity(include_row_ids.serialized_size());
8193
include_row_ids
8294
.serialize_into(&mut filter_bytes)
@@ -91,16 +103,55 @@ impl<R: SeekRead + 'static> FullTextArchiveReader<R> {
91103
}
92104
}
93105

94-
impl FullTextArchiveReader<SliceReader> {
106+
impl FullTextArchiveReader<BytesReader> {
95107
/// Read the whole archive from `input` into memory and open the engine
96-
/// reader over it. Archives are single index files, so a whole-read keeps
97-
/// peak memory at one archive.
108+
/// reader over it. The bytes returned by `input.read()` are held directly
109+
/// by a [`BytesReader`], so no second copy is made and peak memory stays at
110+
/// roughly one archive.
98111
///
99112
/// This is a convenience wrapper over [`from_seek_read`](Self::from_seek_read)
100-
/// for the common whole-file case.
113+
/// for the common whole-file case. For large or remote archives that should
114+
/// not be fully buffered, call [`from_seek_read`](Self::from_seek_read) with
115+
/// a range-reading [`SeekRead`] implementation instead.
101116
pub async fn from_input_file(input: &InputFile) -> crate::Result<Self> {
102117
let bytes = input.read().await?;
103-
Self::from_seek_read(SliceReader::new(bytes.to_vec()))
118+
Self::from_seek_read(BytesReader::new(bytes))
119+
}
120+
}
121+
122+
/// A whole-archive [`SeekRead`] backed by an in-memory [`Bytes`] buffer.
123+
///
124+
/// Unlike the core `SliceReader` (which owns a `Vec<u8>`), this holds the
125+
/// `Bytes` returned by `InputFile::read()` directly, so opening a reader from a
126+
/// whole-file read does not copy the archive a second time.
127+
pub struct BytesReader {
128+
data: Bytes,
129+
}
130+
131+
impl BytesReader {
132+
/// Wrap an already-read archive buffer.
133+
pub fn new(data: Bytes) -> Self {
134+
Self { data }
135+
}
136+
}
137+
138+
impl SeekRead for BytesReader {
139+
fn pread(&self, ranges: &mut [ReadRequest<'_>]) -> io::Result<()> {
140+
for range in ranges {
141+
let start = usize::try_from(range.pos)
142+
.map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "offset overflow"))?;
143+
let end = start
144+
.checked_add(range.buf.len())
145+
.ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "range overflow"))?;
146+
if end > self.data.len() {
147+
return Err(io::Error::new(
148+
io::ErrorKind::UnexpectedEof,
149+
"read past end of archive",
150+
));
151+
}
152+
range.buf.copy_from_slice(&self.data[start..end]);
153+
}
154+
Ok(())
104155
}
105156
}
106157

@@ -115,7 +166,6 @@ fn map_ft_err(e: paimon_ftindex_core::FtIndexError) -> crate::Error {
115166
mod tests {
116167
use super::*;
117168
use crate::io::FileIOBuilder;
118-
use bytes::Bytes;
119169
use paimon_ftindex_core::io::PosWriter;
120170
use paimon_ftindex_core::{FullTextIndexConfig, FullTextIndexWriter};
121171

@@ -175,7 +225,7 @@ mod tests {
175225
include.insert(2);
176226

177227
let hits = reader
178-
.search_with_include(r#"{"match":{"query":"token"}}"#, 10, include)
228+
.search_with_include(r#"{"match":{"query":"token"}}"#, 10, &include)
179229
.unwrap();
180230
let mut ids = hits.row_ids.clone();
181231
ids.sort_unstable();
@@ -197,7 +247,7 @@ mod tests {
197247
// Empty allow-list: an empty include-set admits no rows (not all rows).
198248
let empty = roaring::RoaringTreemap::new();
199249
let hits = reader
200-
.search_with_include(r#"{"match":{"query":"token"}}"#, 10, empty)
250+
.search_with_include(r#"{"match":{"query":"token"}}"#, 10, &empty)
201251
.unwrap();
202252
assert!(
203253
hits.row_ids.is_empty(),
@@ -208,11 +258,25 @@ mod tests {
208258
let mut disjoint = roaring::RoaringTreemap::new();
209259
disjoint.insert(99);
210260
let hits = reader
211-
.search_with_include(r#"{"match":{"query":"token"}}"#, 10, disjoint)
261+
.search_with_include(r#"{"match":{"query":"token"}}"#, 10, &disjoint)
212262
.unwrap();
213263
assert!(
214264
hits.row_ids.is_empty(),
215265
"include-set disjoint from matches must admit no rows"
216266
);
217267
}
268+
269+
#[test]
270+
fn test_from_seek_read_opens_archive_directly() {
271+
// Exercise the generic constructor directly (the key API the remote
272+
// FileIO path in #571 depends on), bypassing `from_input_file`.
273+
let bytes = build_archive(&[(0, "alpha bravo"), (1, "bravo charlie")]);
274+
let reader = FullTextArchiveReader::from_seek_read(BytesReader::new(Bytes::from(bytes)))
275+
.unwrap();
276+
277+
let hits = reader.search(r#"{"match":{"query":"bravo"}}"#, 10).unwrap();
278+
let mut ids = hits.row_ids.clone();
279+
ids.sort_unstable();
280+
assert_eq!(ids, vec![0, 1]);
281+
}
218282
}

0 commit comments

Comments
 (0)