leekeiabstraction commented on code in PR #365:
URL: https://github.com/apache/fluss-rust/pull/365#discussion_r2842767457
##########
crates/fluss/src/row/mod.rs:
##########
@@ -63,58 +65,58 @@ pub trait InternalRow: Send + Sync {
fn is_null_at(&self, pos: usize) -> bool;
/// Returns the boolean value at the given position
- fn get_boolean(&self, pos: usize) -> bool;
+ fn get_boolean(&self, pos: usize) -> Result<bool>;
Review Comment:
Hmm, I've looked at ways to retain current trait signature (not wrapping
with Result<>) but being able to perform column type check on upsert / append.
The options are:
1. Add `fn validate(&self, _row_type: &RowType) -> Result<()>` on
`InternalRow` trait. GenericRow will implement column type checking within this
function.
- This is not ideal Java side does not have this, users can also see /
call the function. Any additional function that we add here needs to be
backward compatible in the future so we cannot remove or change this function's
signature without new major version.
- I would not recommend this
1. Narrow down `append()` and `upsert()` e.g. `pub fn upsert<R:
GenericRow>(&self, row: &R) -> Result<WriteResultFuture>`. GenericRow to have
a `pub (crate)` validate function.
- User will not be able to see or call the validate function which is
great.
- The drawback here is that we have tighten down the type of rows we can
accept from `InternalRow` to `GenericRow`. I am not sure if user expects to be
able append / upsert a `CompactedRow` or `ColumnarRow`, both of which are
`fluss-rust` generated, if we want to serve that use case, tightening to accept
GenericRow only is a no-no.
- the impact on API evolution here is smaller than 1. because widening
the accepted type is backward compatible
1. Define `try_get_*()` on internal row. Postgres does similar thing and the
non-panic-ing function can be used to prevent panic on upsert / append.
- Java side does not have `try_get_*()`, but I'd argue that this is an
improvement of user experience
I'll hold off from further revision until we make a decision on this
actually. Important to get this right. Personally I do prefer fully safe
accessors, rust support for dealing with Results are quite good anyway (but I
am not a heavy rust user outside of fluss-rust!).
Postgres example for 3.
```rust
// Panics on wrong type or missing column
pub fn get<'a, I, T>(&'a self, idx: I) -> T { ... }
// Returns Result on wrong type or missing column
pub fn try_get<'a, I, T>(&'a self, idx: I) -> Result<T, Error> { ...
}
//Both delegate to the same get_inner:
fn get_inner<'a, I, T>(&'a self, idx: &I) -> Result<T, Error> {
// 1. Resolve column index (by name or position)
let idx = match idx.__idx(self.columns()) {
Some(idx) => idx,
None => return Err(Error::column(idx.to_string())),
};
// 2. Check type compatibility BEFORE deserializing
let ty = self.columns()[idx].type_();
if !T::accepts(ty) {
return Err(Error::from_sql(
Box::new(WrongType::new::<T>(ty.clone())),
idx,
));
}
// 3. Only then deserialize
FromSql::from_sql_nullable(ty, self.col_buffer(idx))
.map_err(|e| Error::from_sql(e, idx))
}
//Then get wraps it with a panic:
pub fn get<'a, I, T>(&'a self, idx: I) -> T {
match self.get_inner(&idx) {
Ok(ok) => ok,
Err(err) => panic!("error retrieving column {}: {}", idx,
err),
```
>
--
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]
