Copilot commented on code in PR #238:
URL: https://github.com/apache/fluss-rust/pull/238#discussion_r2763500659
##########
crates/fluss/src/client/table/mod.rs:
##########
@@ -140,8 +150,106 @@ impl<'a> FlussTable<'a> {
}
}
+fn validate_scan_support(table_path: &TablePath, table_info: &TableInfo) ->
Result<()> {
+ if table_info.schema.primary_key().is_some() {
+ return Err(UnsupportedOperation {
+ message: format!("Table {table_path} is not a Log Table and
doesn't support scan."),
Review Comment:
The error message structure is slightly inconsistent with similar error
messages in the codebase. For example, the error at line 141 follows the
pattern "Operation is only supported for X" without mentioning the table.
Consider aligning the message format to either: "Scan is only supported for Log
Tables without primary keys. Table {table_path} has a primary key." This would
be more consistent with the format used at line 164.
```suggestion
message: format!(
"Scan is only supported for Log Tables without primary keys.
Table {table_path} has a primary key."
),
```
##########
crates/fluss/tests/integration/log_table.rs:
##########
@@ -546,7 +557,11 @@ mod table_test {
// Test 4: Subscribing from mid-offset should truncate batch (Arrow
batch slicing)
// Server returns all records from start of batch, but client
truncates to subscription offset
- let trunc_scanner =
table.new_scan().create_record_batch_log_scanner().unwrap();
+ let trunc_scanner = table
+ .new_scan()
+ .unwrap()
Review Comment:
Inconsistent error handling: `.unwrap()` is used here while `.expect("Failed
to create table scan")` is used elsewhere in the same file (e.g., lines 122,
161, 405, 415, 428, 476, 1119). For consistency and better debugging
information when tests fail, consider using `.expect("Failed to create table
scan")` instead.
##########
crates/fluss/src/client/table/mod.rs:
##########
@@ -72,8 +73,17 @@ impl<'a> FlussTable<'a> {
))
}
Review Comment:
Consider adding documentation to the `new_scan` method to explain its
purpose, return value, and error conditions. This would be particularly helpful
since the signature has changed to return a Result. The documentation should
mention that it fails for tables with primary keys or tables that don't use
ARROW log format. This would follow the pattern used by `new_lookup` method at
line 105.
```suggestion
/// Creates a new `TableScan` for configuring scan (read) operations.
///
/// This follows the same pattern as `new_append()` and `new_lookup()`,
/// returning a configuration object that can be used to create a
`Scanner`.
///
/// The table must **not** have a primary key, and it must use the ARROW
/// log format. Scans are only supported for non-primary-key tables that
/// store their logs in ARROW format.
///
/// # Returns
/// * `Ok(TableScan)` - A scan configuration object
/// * `Err(Error)` - If the table has a primary key or does not use the
/// ARROW log format
///
/// # Example
/// ```ignore
/// let table = conn.get_table(&table_path).await?;
/// let scanner = table.new_scan()?.create_scanner()?;
/// let mut batches = scanner.scan().await?;
/// while let Some(batch) = batches.next().await {
/// let batch = batch?;
/// println!("Read batch with {} rows", batch.num_rows());
/// }
/// ```
```
##########
crates/fluss/tests/integration/log_table.rs:
##########
@@ -570,6 +585,7 @@ mod table_test {
// Test 5: Projection should only return requested columns
let proj = table
.new_scan()
+ .unwrap()
Review Comment:
Inconsistent error handling: `.unwrap()` is used here while `.expect("Failed
to create table scan")` is used elsewhere in the same file (e.g., lines 122,
161, 405, 415, 428, 476, 1119). For consistency and better debugging
information when tests fail, consider using `.expect("Failed to create table
scan")` instead.
```suggestion
.expect("Failed to create table scan")
```
--
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]
