Skip to content
Merged
Changes from 2 commits
Commits
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
32 changes: 32 additions & 0 deletions crates/fluss/src/client/table/lookup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,16 @@ 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::ArrowRecordBatchInnerBuilder;

@fresh-borzoni fresh-borzoni Mar 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we use it?

If we use trait import just to make trait method to work, it might be a better idea to expose a helper method in RowAppendRecordBatchBuilder

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This import is still required. In Rust, trait methods can only be called if the trait is in scope. RowAppendRecordBatchBuilder implements ArrowRecordBatchInnerBuilder, so without this import, .append() and .build_arrow_record_batch() are not callable. Removing it breaks the build.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I understand the trait needs to be in scope, my concern is that ArrowRecordBatchInnerBuilder is an internal trait leaking into this module.

Could you add inherent methods on RowAppendRecordBatchBuilder that delegate to the trait methods?
Then the trait stays internal and this module just calls builder.append() / builder.build_arrow_record_batch() without the import.

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.
Expand Down Expand Up @@ -85,6 +88,35 @@ impl LookupResult {
.map(|bytes| CompactedRow::from_bytes(&self.row_type, &bytes[SCHEMA_ID_LENGTH..]))
.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> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it's a new user facing API, shall we add tests and docs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done added!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what about bindings?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let's leave the bindings out of scope for now
probably we wish to have just to_arrow on lookup result, but better in followup PR

let mut builder = RowAppendRecordBatchBuilder::new(&self.row_type)?;

for bytes in &self.rows {
let payload = bytes.get(SCHEMA_ID_LENGTH..).ok_or_else(|| {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do you mind to also change get_single_row() and get_rows() to the same pattern?
or it's inconsistent with this change

Error::RowConvertError {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

different error types for the same schema check - let's use RowConvertError then in other places

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

message: format!(
"LookupResult row payload too short: {} bytes, need at least {} bytes for schema id",
bytes.len(),
SCHEMA_ID_LENGTH
),
}
})?;

let row = CompactedRow::from_bytes(&self.row_type, payload);
builder.append(&row)?;
Comment on lines +122 to +126

Copilot AI Mar 3, 2026

Copy link

Choose a reason for hiding this comment

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

&bytes[SCHEMA_ID_LENGTH..] will panic if a returned row payload is shorter than SCHEMA_ID_LENGTH (e.g., corrupted/partial response). Since this is a new public API, it would be better to avoid panicking here and instead return a structured Error by using a checked slice (bytes.get(SCHEMA_ID_LENGTH..)) and failing gracefully if it’s missing.

Copilot uses AI. Check for mistakes.
}

let arc_batch = builder.build_arrow_record_batch()?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

builder.build_arrow_record_batch().map(Arc::unwrap_or_clone)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Ok(Arc::unwrap_or_clone(arc_batch))
}
}

/// Configuration and factory struct for creating lookup operations.
Expand Down