leekeiabstraction commented on code in PR #144:
URL: https://github.com/apache/fluss-rust/pull/144#discussion_r2680079125
##########
crates/fluss/src/row/binary/binary_writer.rs:
##########
@@ -52,14 +52,11 @@ pub trait BinaryWriter {
fn write_binary(&mut self, bytes: &[u8], length: usize);
- // TODO Decimal type
- // fn write_decimal(&mut self, pos: i32, value: f64);
+ fn write_decimal(&mut self, value: &rust_decimal::Decimal, precision: u32);
Review Comment:
nit: `decimal` as name should suffice, none other args within this trait
starts with `rust_`
##########
crates/fluss/src/row/compacted/compacted_row_writer.rs:
##########
@@ -153,4 +153,21 @@ impl CompactedRowWriter {
pub fn write_double(&mut self, value: f64) {
self.write_raw(&value.to_ne_bytes());
}
+
+ pub fn write_decimal(&mut self, value: &rust_decimal::Decimal, _precision:
u32) {
+ // For now, serialize decimal to its string representation and write
as bytes
+ // TODO: implement compact decimal encoding based on precision similar
to Java implementation
+ let s = value.to_string();
+ self.write_bytes(s.as_bytes());
+ }
+
+ pub fn write_timestamp_ntz(&mut self, value: i64, _precision: u32) {
+ // Currently write timestamp as a long (epoch millis or other unit
depending on upstream)
+ self.write_long(value);
Review Comment:
It might be better to leave this as TODO instead of having an implementation
that differs from Java.
##########
crates/fluss/src/row/compacted/compacted_row_writer.rs:
##########
@@ -153,4 +153,21 @@ impl CompactedRowWriter {
pub fn write_double(&mut self, value: f64) {
self.write_raw(&value.to_ne_bytes());
}
+
+ pub fn write_decimal(&mut self, value: &rust_decimal::Decimal, _precision:
u32) {
+ // For now, serialize decimal to its string representation and write
as bytes
+ // TODO: implement compact decimal encoding based on precision similar
to Java implementation
+ let s = value.to_string();
+ self.write_bytes(s.as_bytes());
Review Comment:
It might be better to leave this as TODO instead of having an implementation
that differs from Java.
##########
crates/fluss/src/row/compacted/compacted_row_writer.rs:
##########
@@ -153,4 +153,21 @@ impl CompactedRowWriter {
pub fn write_double(&mut self, value: f64) {
self.write_raw(&value.to_ne_bytes());
}
+
+ pub fn write_decimal(&mut self, value: &rust_decimal::Decimal, _precision:
u32) {
+ // For now, serialize decimal to its string representation and write
as bytes
+ // TODO: implement compact decimal encoding based on precision similar
to Java implementation
+ let s = value.to_string();
+ self.write_bytes(s.as_bytes());
+ }
+
+ pub fn write_timestamp_ntz(&mut self, value: i64, _precision: u32) {
+ // Currently write timestamp as a long (epoch millis or other unit
depending on upstream)
+ self.write_long(value);
+ }
+
+ pub fn write_timestamp_ltz(&mut self, value: i64, _precision: u32) {
+ // Currently write timestamp as a long (epoch millis or other unit
depending on upstream)
+ self.write_long(value);
Review Comment:
It might be better to leave this as TODO instead of having an implementation
that differs from Java.
##########
crates/fluss/src/row/encode/compacted_key_encoder.rs:
##########
@@ -270,14 +270,13 @@ mod tests {
Datum::from(-6101065172474983726i64), // from Java test case: new
BigInteger("12345678901234567890").longValue()
Datum::from(13.2f32),
Datum::from(15.21f64),
- // TODO Date
- // TODO Time
+ Datum::Date(crate::row::datum::Date::new(5)),
Review Comment:
What does 5 mean?
I suggest using the date 2023-10-25 to keep test case parity with java side.
Ref here:
https://github.com/apache/fluss/blob/2b0e2b1b08ede61874e71e21877134d8945fe8c8/fluss-common/src/test/java/org/apache/fluss/row/indexed/IndexedRowTest.java#L196
##########
crates/fluss/src/row/encode/compacted_key_encoder.rs:
##########
@@ -270,14 +270,13 @@ mod tests {
Datum::from(-6101065172474983726i64), // from Java test case: new
BigInteger("12345678901234567890").longValue()
Datum::from(13.2f32),
Datum::from(15.21f64),
- // TODO Date
- // TODO Time
+ Datum::Date(crate::row::datum::Date::new(5)),
+ Datum::Timestamp(crate::row::datum::Timestamp::new(13)),
Review Comment:
I suggest using 09:30:00.0 to keep test case parity with java side.
Ref here:
https://github.com/apache/fluss/blob/2b0e2b1b08ede61874e71e21877134d8945fe8c8/fluss-common/src/test/java/org/apache/fluss/row/indexed/IndexedRowTest.java#L198
--
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]
