leekeiabstraction commented on code in PR #138:
URL: https://github.com/apache/fluss-rust/pull/138#discussion_r2678955421
##########
crates/fluss/src/row/datum.rs:
##########
@@ -296,9 +292,7 @@ impl Datum<'_> {
Datum::Float32(v) => append_value_to_arrow!(Float32Builder,
v.into_inner()),
Datum::Float64(v) => append_value_to_arrow!(Float64Builder,
v.into_inner()),
Datum::String(v) => append_value_to_arrow!(StringBuilder, *v),
- Datum::OwnedString(v) => append_value_to_arrow!(StringBuilder,
v.as_str()),
- Datum::Blob(v) => append_value_to_arrow!(BinaryBuilder,
v.as_ref()),
- Datum::BorrowedBlob(v) => append_value_to_arrow!(BinaryBuilder,
*v),
Review Comment:
Thanks for cleaning this up!
I've also raised a PR that allows for creating Datum out of both borrowed
and owned String/Blob without bloat to enum. However, it seems that you have
gotten rid of places where we actually create Datum out of owned data.
If we do not have or expect use-case of creating Datum out of owned
string/blob, I can discard my PR. Otherwise I am happy to resolve conflict in
my PR after yours is merged.
https://github.com/apache/fluss-rust/pull/139
##########
crates/fluss/src/row/compacted/compacted_row.rs:
##########
@@ -100,11 +101,29 @@ impl CompactedRow {
(self.segment[idx] & (1u8 << bit)) != 0
}
- fn decoded_row(&mut self) -> &GenericRow<'static> {
+ fn decoded_row(&mut self) -> &GenericRow<'a> {
if !self.decoded {
+ let deserializer =
CompactedRowDeserializer::new(self.data_types.clone());
Review Comment:
Is it possible to have CompactedRowDeserializer to contain `&[DataType]` so
that we can skip this clone?
```
pub struct CompactedRowDeserializer<'a> {
schema: &'a[DataType],
}
impl <'a> CompactedRowDeserializer<'a> {
...
pub fn deserialize<'b>(&self, reader: &'b CompactedRowReader) ->
GenericRow<'b> {
...
}
}
```
```
let deserializer = CompactedRowDeserializer::new(self.data_types.as_slice());
```
Nit: we could consider initialising deserialiser higher up the stack (pass
in deserialiser instead of &[DataType]) when building CompactedRow to avoid
cost of reconstructing deserialisers.
--
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]
