-
Notifications
You must be signed in to change notification settings - Fork 49
[client] Add LookupResult::to_record_batch() #411
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
Changes from all commits
31f4fcf
c158c4f
aadbc3f
9019713
6207505
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 |
|---|---|---|
|
|
@@ -20,13 +20,15 @@ use crate::client::metadata::Metadata; | |
| use crate::client::table::partition_getter::PartitionGetter; | ||
| use crate::error::{Error, Result}; | ||
| use crate::metadata::{PhysicalTablePath, RowType, TableBucket, TableInfo, TablePath}; | ||
| use crate::record::RowAppendRecordBatchBuilder; | ||
| use crate::record::kv::SCHEMA_ID_LENGTH; | ||
| use crate::row::InternalRow; | ||
| use crate::row::compacted::CompactedRow; | ||
| use crate::row::encode::{KeyEncoder, KeyEncoderFactory}; | ||
| use crate::rpc::ApiError; | ||
| use crate::rpc::RpcClient; | ||
| use crate::rpc::message::LookupRequest; | ||
| use arrow::array::RecordBatch; | ||
| use std::sync::Arc; | ||
|
|
||
| /// The result of a lookup operation. | ||
|
|
@@ -53,6 +55,19 @@ impl LookupResult { | |
| } | ||
| } | ||
|
|
||
| /// Extracts the row payload by stripping the schema id prefix. | ||
| fn extract_payload(bytes: &[u8]) -> Result<&[u8]> { | ||
| bytes | ||
| .get(SCHEMA_ID_LENGTH..) | ||
| .ok_or_else(|| Error::RowConvertError { | ||
| message: format!( | ||
| "Row payload too short: {} bytes, need at least {} for schema id", | ||
| bytes.len(), | ||
| SCHEMA_ID_LENGTH | ||
| ), | ||
| }) | ||
| } | ||
|
|
||
| /// Returns the only row in the result set as a [`CompactedRow`]. | ||
| /// | ||
| /// This method provides a zero-copy view of the row data, which means the returned | ||
|
|
@@ -62,29 +77,57 @@ impl LookupResult { | |
| /// - `Ok(Some(row))`: If exactly one row exists. | ||
| /// - `Ok(None)`: If the result set is empty. | ||
| /// - `Err(Error::UnexpectedError)`: If the result set contains more than one row. | ||
| /// | ||
| /// - `Err(Error)`: If the row payload is too short to contain a schema id. | ||
| pub fn get_single_row(&self) -> Result<Option<CompactedRow<'_>>> { | ||
| match self.rows.len() { | ||
| 0 => Ok(None), | ||
| 1 => Ok(Some(CompactedRow::from_bytes( | ||
| &self.row_type, | ||
| &self.rows[0][SCHEMA_ID_LENGTH..], | ||
| ))), | ||
| 1 => { | ||
| let payload = Self::extract_payload(&self.rows[0])?; | ||
| Ok(Some(CompactedRow::from_bytes(&self.row_type, payload))) | ||
| } | ||
| _ => Err(Error::UnexpectedError { | ||
| message: "LookupResult contains multiple rows, use get_rows() instead".to_string(), | ||
| source: None, | ||
| }), | ||
| } | ||
| } | ||
|
|
||
| /// Returns all rows as CompactedRows. | ||
| pub fn get_rows(&self) -> Vec<CompactedRow<'_>> { | ||
| /// Returns all rows in the result set as [`CompactedRow`]s. | ||
| /// | ||
| /// # Returns | ||
| /// - `Ok(rows)` - All rows in the result set. | ||
| /// - `Err(Error)` - If any row payload is too short to contain a schema id. | ||
| pub fn get_rows(&self) -> Result<Vec<CompactedRow<'_>>> { | ||
|
Member
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. we change public API, shall we update docs?
Contributor
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 think the change is fine since it hasn't exposed to users. |
||
| self.rows | ||
| .iter() | ||
| // TODO Add schema id check and fetch when implementing prefix lookup | ||
|
Member
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. this comment should stay
Contributor
Author
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. sorry, added |
||
| .map(|bytes| CompactedRow::from_bytes(&self.row_type, &bytes[SCHEMA_ID_LENGTH..])) | ||
| .map(|bytes| { | ||
| let payload = Self::extract_payload(bytes)?; | ||
| Ok(CompactedRow::from_bytes(&self.row_type, payload)) | ||
| }) | ||
| .collect() | ||
| } | ||
|
|
||
| /// Converts all rows in this result into an Arrow [`RecordBatch`]. | ||
| /// | ||
| /// This is useful for integration with DataFusion or other Arrow-based tools. | ||
| /// | ||
| /// # Returns | ||
| /// - `Ok(RecordBatch)` - All rows in columnar Arrow format. Returns an empty | ||
| /// batch (with the correct schema) if the result set is empty. | ||
| /// - `Err(Error)` - If the conversion fails. | ||
| pub fn to_record_batch(&self) -> Result<RecordBatch> { | ||
|
Member
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. what about bindings?
Contributor
Author
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. Could you clarify what you'd like here? The core to_record_batch() method is available on LookupResult. For Python, we could add a lookup_arrow() method on Lookuper that returns a PyArrow RecordBatch — but I wanted to confirm the expected API shape before adding it.
Member
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. let's leave the bindings out of scope for now |
||
| let mut builder = RowAppendRecordBatchBuilder::new(&self.row_type)?; | ||
|
|
||
| for bytes in &self.rows { | ||
| let payload = Self::extract_payload(bytes)?; | ||
|
|
||
| let row = CompactedRow::from_bytes(&self.row_type, payload); | ||
| builder.append(&row)?; | ||
| } | ||
|
|
||
| builder.build_arrow_record_batch().map(Arc::unwrap_or_clone) | ||
| } | ||
| } | ||
|
|
||
| /// Configuration and factory struct for creating lookup operations. | ||
|
|
@@ -306,3 +349,68 @@ impl Lookuper { | |
| &self.table_info | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use crate::metadata::{DataField, DataTypes}; | ||
| use crate::row::binary::BinaryWriter; | ||
| use crate::row::compacted::CompactedRowWriter; | ||
| use arrow::array::Int32Array; | ||
|
|
||
| fn make_row_bytes(schema_id: i16, row_data: &[u8]) -> Vec<u8> { | ||
| let mut bytes = Vec::with_capacity(SCHEMA_ID_LENGTH + row_data.len()); | ||
| bytes.extend_from_slice(&schema_id.to_le_bytes()); | ||
| bytes.extend_from_slice(row_data); | ||
| bytes | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_to_record_batch_empty() { | ||
| let row_type = Arc::new(RowType::new(vec![DataField::new( | ||
| "id", | ||
| DataTypes::int(), | ||
| None, | ||
| )])); | ||
| let result = LookupResult::empty(row_type); | ||
| let batch = result.to_record_batch().unwrap(); | ||
| assert_eq!(batch.num_rows(), 0); | ||
| assert_eq!(batch.num_columns(), 1); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_to_record_batch_with_row() { | ||
| let row_type = Arc::new(RowType::new(vec![DataField::new( | ||
| "id", | ||
| DataTypes::int(), | ||
| None, | ||
| )])); | ||
|
|
||
| let mut writer = CompactedRowWriter::new(1); | ||
| writer.write_int(42); | ||
| let row_bytes = make_row_bytes(0, writer.buffer()); | ||
|
|
||
| let result = LookupResult::new(vec![row_bytes], Arc::clone(&row_type)); | ||
| let batch = result.to_record_batch().unwrap(); | ||
|
|
||
| assert_eq!(batch.num_rows(), 1); | ||
| let col = batch | ||
| .column(0) | ||
| .as_any() | ||
| .downcast_ref::<Int32Array>() | ||
| .unwrap(); | ||
| assert_eq!(col.value(0), 42); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_to_record_batch_payload_too_short() { | ||
| let row_type = Arc::new(RowType::new(vec![DataField::new( | ||
| "id", | ||
| DataTypes::int(), | ||
| None, | ||
| )])); | ||
| // Only 1 byte — shorter than SCHEMA_ID_LENGTH (2) | ||
| let result = LookupResult::new(vec![vec![0u8]], Arc::clone(&row_type)); | ||
| assert!(result.to_record_batch().is_err()); | ||
| } | ||
| } | ||
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.
why did we remove this?
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.
added