Copilot commented on code in PR #105:
URL: https://github.com/apache/fluss-rust/pull/105#discussion_r2637545843
##########
crates/fluss/src/row/mod.rs:
##########
@@ -51,7 +51,7 @@ pub trait InternalRow {
fn get_double(&self, pos: usize) -> f64;
/// Returns the string value at the given position with fixed length
- fn get_char(&self, pos: usize, length: usize) -> String;
+ fn get_char(&self, pos: usize, length: usize) -> &str;
Review Comment:
Changing the return type from `String` to `&str` is a breaking API change
for the `InternalRow` trait. This will break any existing code that calls
`get_char()` and expects to own the returned string. Consider whether this
breaking change is intentional and properly communicated to users of this API.
```suggestion
fn get_char(&self, pos: usize, length: usize) -> String;
```
##########
crates/fluss/src/row/mod.rs:
##########
@@ -116,16 +116,9 @@ impl<'a> InternalRow for GenericRow<'a> {
self.values.get(pos).unwrap().try_into().unwrap()
}
- fn get_char(&self, pos: usize, length: usize) -> String {
- let value = self.get_string(pos);
- if value.len() != length {
- panic!(
- "Length mismatch for fixed-size char: expected {}, got {}",
- length,
- value.len()
- );
- }
- value.to_string()
+ fn get_char(&self, pos: usize, _length: usize) -> &str {
+ // don't check length, following java client
+ self.get_string(pos)
Review Comment:
The removal of length validation changes the behavior of this method.
Previously, the method would panic if the string length didn't match the
expected `length` parameter. Now the `length` parameter is ignored. This could
mask data corruption or schema mismatches that would have been caught earlier.
If this behavior change is intentional to match the Java client, consider
documenting this in the method's documentation or adding a comment explaining
why validation was removed.
```suggestion
// NOTE: We intentionally do not enforce the expected `length` here.
// Historically this method would panic if the actual string length
// did not match the `length` parameter, but that strict validation
// was removed to align the Rust client's behavior with the Java
client,
// which ignores the length and simply returns the underlying string.
self.get_string(pos)
```
##########
crates/fluss/src/row/mod.rs:
##########
@@ -116,16 +116,9 @@ impl<'a> InternalRow for GenericRow<'a> {
self.values.get(pos).unwrap().try_into().unwrap()
}
- fn get_char(&self, pos: usize, length: usize) -> String {
- let value = self.get_string(pos);
- if value.len() != length {
- panic!(
- "Length mismatch for fixed-size char: expected {}, got {}",
- length,
- value.len()
- );
- }
- value.to_string()
+ fn get_char(&self, pos: usize, _length: usize) -> &str {
+ // don't check length, following java client
+ self.get_string(pos)
Review Comment:
There is an extra leading space before `self.get_string(pos)`. The line
should be indented consistently with other method implementations in this trait.
```suggestion
self.get_string(pos)
```
##########
crates/fluss/src/row/column.rs:
##########
@@ -135,16 +135,8 @@ impl InternalRow for ColumnarRow {
.expect("Expected fixed-size binary array for char type");
let bytes = array.value(self.row_id);
- if bytes.len() != length {
- panic!(
- "Length mismatch for fixed-size char: expected {}, got {}",
- length,
- bytes.len()
- );
- }
-
- String::from_utf8(bytes.to_vec())
- .unwrap_or_else(|_| String::from_utf8_lossy(bytes).into_owned())
+ // don't check length, following java client
+ std::str::from_utf8(bytes).expect("Invalid UTF-8 in char field")
Review Comment:
The error handling behavior has changed. The old implementation used
`String::from_utf8_lossy()` as a fallback for invalid UTF-8, which would
replace invalid sequences with the replacement character (�). The new
implementation uses `expect()` which will panic on invalid UTF-8. This is a
breaking behavior change that could cause runtime crashes where the code
previously handled invalid UTF-8 gracefully. Consider whether this stricter
error handling is intentional.
--
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]
