fresh-borzoni commented on code in PR #175:
URL: https://github.com/apache/fluss-rust/pull/175#discussion_r2702323114
##########
crates/fluss/src/row/compacted/compacted_row_writer.rs:
##########
@@ -89,64 +108,250 @@ impl BinaryWriter for CompactedRowWriter {
self.buffer[byte_index] |= 1u8 << bit;
}
- fn write_boolean(&mut self, value: bool) {
+ fn write_boolean(&mut self, value: bool) -> Result<()> {
let b = if value { 1u8 } else { 0u8 };
- self.write_raw(&[b]);
+ self.write_raw(&[b])
}
- fn write_byte(&mut self, value: u8) {
- self.write_raw(&[value]);
+ fn write_byte(&mut self, value: u8) -> Result<()> {
+ self.write_raw(&[value])
}
- fn write_bytes(&mut self, value: &[u8]) {
- let len_i32 =
- i32::try_from(value.len()).expect("byte slice too large to encode
length as i32");
- self.write_int(len_i32);
- self.write_raw(value);
+ fn write_bytes(&mut self, value: &[u8]) -> Result<()> {
+ let len_i32 = i32::try_from(value.len()).map_err(|_|
Error::IllegalArgument {
+ message: format!(
+ "Byte slice too large to encode length as i32: {} bytes
exceeds i32::MAX",
+ value.len()
+ ),
+ })?;
+ self.write_int(len_i32)?;
+ self.write_raw(value)
}
- fn write_char(&mut self, value: &str, _length: usize) {
+ fn write_char(&mut self, value: &str, _length: usize) -> Result<()> {
// TODO: currently, we encoding CHAR(length) as the same with STRING,
the length info can be
// omitted and the bytes length should be enforced in the future.
- self.write_string(value);
+ self.write_string(value)
}
- fn write_string(&mut self, value: &str) {
- self.write_bytes(value.as_ref());
+ fn write_string(&mut self, value: &str) -> Result<()> {
+ self.write_bytes(value.as_ref())
}
- fn write_short(&mut self, value: i16) {
- self.write_raw(&value.to_ne_bytes());
+ fn write_short(&mut self, value: i16) -> Result<()> {
+ // Use native endianness to match Java's UnsafeUtils.putShort behavior
+ // Java uses sun.misc.Unsafe which writes in native byte order
(typically LE on x86/ARM)
+ self.write_raw(&value.to_ne_bytes())
}
- fn write_int(&mut self, value: i32) {
+ fn write_int(&mut self, value: i32) -> Result<()> {
self.ensure_capacity(Self::MAX_INT_SIZE);
let bytes_written =
write_unsigned_varint_to_slice(value as u32, &mut
self.buffer[self.position..]);
self.position += bytes_written;
+ Ok(())
}
- fn write_long(&mut self, value: i64) {
+ fn write_long(&mut self, value: i64) -> Result<()> {
self.ensure_capacity(Self::MAX_LONG_SIZE);
let bytes_written =
write_unsigned_varint_u64_to_slice(value as u64, &mut
self.buffer[self.position..]);
self.position += bytes_written;
+ Ok(())
}
- fn write_float(&mut self, value: f32) {
- self.write_raw(&value.to_ne_bytes());
+
+ fn write_float(&mut self, value: f32) -> Result<()> {
+ // Use native endianness to match Java's UnsafeUtils.putFloat behavior
+ self.write_raw(&value.to_ne_bytes())
}
- fn write_double(&mut self, value: f64) {
- self.write_raw(&value.to_ne_bytes());
+ fn write_double(&mut self, value: f64) -> Result<()> {
+ // Use native endianness to match Java's UnsafeUtils.putDouble behavior
+ self.write_raw(&value.to_ne_bytes())
}
- fn write_binary(&mut self, bytes: &[u8], length: usize) {
+ fn write_binary(&mut self, bytes: &[u8], length: usize) -> Result<()> {
// TODO: currently, we encoding BINARY(length) as the same with BYTES,
the length info can
// be omitted and the bytes length should be enforced in the future.
- self.write_bytes(&bytes[..length.min(bytes.len())]);
+ self.write_bytes(&bytes[..length.min(bytes.len())])
}
fn complete(&mut self) {
// do nothing
}
+
+ fn write_decimal(
+ &mut self,
+ value: &bigdecimal::BigDecimal,
+ precision: u32,
+ scale: u32,
+ ) -> Result<()> {
+ const MAX_COMPACT_PRECISION: u32 = 18;
+ use bigdecimal::rounding::RoundingMode;
+
+ // Step 1 (Java: Decimal.fromBigDecimal): rescale + validate precision
+ // bd = bd.setScale(scale, RoundingMode.HALF_UP);
+ let scaled = value.with_scale_round(scale as i64,
RoundingMode::HalfUp);
+
+ // In bigdecimal, after scaling, the "unscaled value" is exactly the
bigint mantissa.
+ // That corresponds to Java:
scaled.movePointRight(scale).toBigIntegerExact().
+ let (unscaled, exp) = scaled.as_bigint_and_exponent();
+
+ // Sanity check
+ debug_assert_eq!(
+ exp, scale as i64,
+ "Scaled decimal exponent ({}) != expected scale ({})",
+ exp, scale
+ );
+
+ // Java validates: if (bd.precision() > precision) return null;
+ // Java BigDecimal.precision() == number of base-10 digits in the
unscaled value (sign ignored),
+ // with 0 having precision 1, and trailing zeros stripped.
+ let prec = java_decimal_precision(&unscaled);
+ if prec > precision as usize {
+ return Err(Error::IllegalArgument {
+ message: format!(
+ "Decimal precision overflow after rescale: scaled={} has
{} digits but precision is {} (orig={})",
+ scaled, prec, precision, value
+ ),
+ });
+ }
Review Comment:
Good callout, yes, now looking with the fresh eye, I think it's better
design in the end 👍
--
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]
