AVRO-3912: Fix Big decimal deser #2599

clesaec · 2023-11-28T09:43:43Z

What is the purpose of the change

AVRO-3912 : fix big decimal deserialisation error (thanks @Ten0 for comment on this PR).

Verifying this change

Unit test from writer to reader in decimal.rs source file

Documentation

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (not applicable)

Ten0 · 2023-11-28T10:16:00Z

I thought the error was on serialization, not deserialization

clesaec · 2023-11-28T10:19:21Z

@Ten0 : I made serialization the same way i made it on Java, to keep capabilities to serialize in Java or Rust and deserialize in Java or Rust. So, i found better to fix deserialization in Rust than to review Java & rust module.

Ten0 · 2023-11-28T10:24:26Z

If you skip serializing the bytes length and you still have the scale after the first buffer then that means the BigDecimal logical type cannot be deserialized as bytes (because you'd have an extra int after), which looks like it's incorrect because it's only a logical type and not a full new type. (One would expect from a logical type that if it's unknown by their library they can still deserialize it as the main type and either ignore it or interprete it manually.)
Also IIUC the Java implementation starts by deserializing as bytes and then converts that into a BigDecimal, so we do have the two lengths.

martin-g · 2023-11-28T13:21:54Z

lang/rust/avro/src/decimal.rs

@@ -214,4 +218,63 @@ mod tests {

        Ok(())
    }
+
+    #[test]
+    fn other() -> TestResult {


Please give the method a better name, e.g. test_avro_3912_...

martin-g · 2023-11-28T13:22:43Z

lang/rust/avro/src/decimal.rs

+        let val = BigDecimal::new(BigInt::from(12), 2);
+        record.put("field_name", val.clone());
+
+        // save record in writer


Suggested change

// save record in writer

// write a record

martin-g · 2023-11-28T13:23:51Z

lang/rust/avro/src/decimal.rs

+            .build();
+
+        let result = writer.append(record.clone());
+        assert!(result.is_ok(), "Append KO  : {}", result.unwrap_err());


Suggested change

assert!(result.is_ok(), "Append KO : {}", result.unwrap_err());

assert!(result.is_ok(), "Append NOK : {result}");

martin-g · 2023-11-28T13:25:55Z

lang/rust/avro/src/decimal.rs

+
+        let result = writer.append(record.clone());
+        assert!(result.is_ok(), "Append KO  : {}", result.unwrap_err());
+        assert!(writer.flush().is_ok(), "Flush KO");


Actually there is no need to assert!() these above. You could just use ?, e.g. writer.append(record.clone())?; and it will propagate the error because the method returns TestResult.

I try to avoid using .unwrap() in general. Especially in non-test code its usage should be forbidden. I am not sure whether there is a Clippy check that we could enforce.

https://doc.rust-lang.org/book/ch09-03-to-panic-or-not-to-panic.html#cases-in-which-you-have-more-information-than-the-compiler

martin-g · 2023-11-28T13:29:47Z

Let's add an interop test (or extend the current one) for BigDecimal in Rust and Java!

clesaec · 2023-11-28T14:36:46Z

Get it, i didn't realize that a logical type has to be deserializable in its main type; i do the fix.

Ten0 · 2023-11-29T02:35:39Z

By the way if the scale was written at the beginning of the Bytes instead of the end then you wouldn't need to serialize an extra length, because the scale encoded like other ints is self-delimited so then the rest of the Bytes would be the big endian encoded part. That would be a more efficient storage, more in the spirit of the rest of the format which is to store as little amount of metadata as possible. Maybe it isn't too late to change that since it's not released?

clesaec · 2023-11-29T14:28:07Z

I add test with file generated with Java module and fix code comments.

To with scale to avoid serializing an extra length is a good idea, but, Java decoder interface miss a function like "readRemainder()" or something similar; because we don't know the number of byte took by scale value with readInt() (can be one or several byte, depends on the value itself).

Ten0 · 2023-11-29T14:54:29Z

Java decoder interface miss a function like "readRemainder()" or something similar

Isn't .remaining() it?

avro/lang/java/avro/src/main/java/org/apache/avro/Conversions.java

Lines 90 to 93 in b4968f7

    
           public BigDecimal fromBytes(ByteBuffer value, Schema schema, LogicalType type) { 
        
             int scale = ((LogicalTypes.Decimal) type).getScale(); 
        
             // always copy the bytes out because BigInteger has no offset/length ctor 
        
             byte[] bytes = new byte[value.remaining()];

clesaec · 2023-11-29T15:30:26Z

.remaining() method apply on ByteBuffer value variable, while decoder class use its own "pos" field to "iterate" on data. So, it would need as "byte[] decoder.readRemaining()" method.

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

martin-g · 2023-12-01T12:45:58Z

lang/rust/avro/src/decimal.rs

@@ -113,13 +113,17 @@ impl<T: AsRef<[u8]>> From<T> for Decimal {
 }

 pub(crate) fn serialize_big_decimal(decimal: &BigDecimal) -> Vec<u8> {
+    // encode big decimal, without global size


global size sounds too ... global.
How about total size ?

…bigdecimal.rs) Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

Ten0 · 2023-12-01T14:49:19Z

.remaining() method apply on ByteBuffer value variable, while decoder class use its own "pos" field to "iterate" on data. So, it would need as "byte[] decoder.readRemaining()" method.

Alternately you could make getPos public:

avro/lang/java/avro/src/main/java/org/apache/avro/io/BinaryDecoder.java

Line 73 in b4968f7

int getPos() {

and then parse value[decoder.getPos()..].
Either way that looks simple enough.

clesaec · 2023-12-01T15:29:00Z

make getPos public => Well, pos is an implementattion detail of BinaryDecorder, that may should not be public.
The need is here, but i think it needs some works and discussion on Java side.
In mean time, can this be acceptable as this ?

clesaec · 2023-12-04T07:56:35Z

I first merge this and report to 1.11 branch to align Rust & Java BigDecimal logical type.
In a second time, i'll propose 2 PR in main branch to optimize it in main branch; then report in 1.11

* AVRO-3912: Fix Big decimal deser Co-authored-by: Martin Tzvetanov Grigorov <[email protected]>

* AVRO-3912: Fix Big decimal deser (#2599) * AVRO-3912: Fix Big decimal deser Co-authored-by: Martin Tzvetanov Grigorov <[email protected]> * AVRO-3912: error comment

* AVRO-3912: Fix Big decimal deser Co-authored-by: Martin Tzvetanov Grigorov <[email protected]>

AVRO-3912: Fix Big decimal deser

9941517

clesaec requested a review from martin-g November 28, 2023 09:43

github-actions bot added the Rust label Nov 28, 2023

clesaec added 2 commits November 28, 2023 11:14

AVRO-3912: Fix import

c4f16ff

AVRO-3912: Fix import

9d9b7e6

martin-g reviewed Nov 28, 2023

View reviewed changes

clesaec marked this pull request as draft November 28, 2023 14:37

clesaec added 2 commits November 29, 2023 10:24

AVRO-3912: with size

b08aa9d

AVRO-3912: add test from file generated with java module

be9c879

clesaec marked this pull request as ready for review November 29, 2023 14:21

clesaec added 2 commits November 29, 2023 15:30

AVRO-3912: fix fmt

50ab564

AVRO-3912: fix fmt

c0ab767

clesaec requested a review from martin-g November 30, 2023 08:09

AVRO-3912: [Rust] Minor cleanups

03940a3

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

martin-g reviewed Dec 1, 2023

View reviewed changes

martin-g added 2 commits December 1, 2023 15:19

AVRO-3912: [Rust] Extract the code for BigDecimal to its own module (…

abb4da4

…bigdecimal.rs) Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

AVRO-3912: Minor cleanup in shared.rs

6d3869b

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

martin-g approved these changes Dec 1, 2023

View reviewed changes

clesaec merged commit 9c4103c into apache:main Dec 4, 2023
15 checks passed

clesaec added a commit to clesaec/avro that referenced this pull request Dec 4, 2023

AVRO-3912: Fix Big decimal deser (apache#2599)

6107c12

* AVRO-3912: Fix Big decimal deser Co-authored-by: Martin Tzvetanov Grigorov <[email protected]>

martin-g pushed a commit that referenced this pull request Dec 4, 2023

AVRO-3912: big dec fix1 11 (#2607)

1b3591d

* AVRO-3912: Fix Big decimal deser (#2599) * AVRO-3912: Fix Big decimal deser Co-authored-by: Martin Tzvetanov Grigorov <[email protected]> * AVRO-3912: error comment

RanbirK pushed a commit to RanbirK/avro that referenced this pull request May 13, 2024

AVRO-3912: Fix Big decimal deser (apache#2599)

f39ff50

* AVRO-3912: Fix Big decimal deser Co-authored-by: Martin Tzvetanov Grigorov <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AVRO-3912: Fix Big decimal deser #2599

AVRO-3912: Fix Big decimal deser #2599

clesaec commented Nov 28, 2023

Ten0 commented Nov 28, 2023

clesaec commented Nov 28, 2023

Ten0 commented Nov 28, 2023 •

edited

Loading

martin-g Nov 28, 2023

martin-g Nov 28, 2023

martin-g Nov 28, 2023

martin-g Nov 28, 2023

Ten0 Nov 28, 2023

martin-g commented Nov 28, 2023

clesaec commented Nov 28, 2023

Ten0 commented Nov 29, 2023 •

edited

Loading

clesaec commented Nov 29, 2023

Ten0 commented Nov 29, 2023

clesaec commented Nov 29, 2023

martin-g Dec 1, 2023

Ten0 commented Dec 1, 2023 •

edited

Loading

clesaec commented Dec 1, 2023

clesaec commented Dec 4, 2023

	assert!(result.is_ok(), "Append KO : {}", result.unwrap_err());
	assert!(result.is_ok(), "Append NOK : {result}");

AVRO-3912: Fix Big decimal deser #2599

AVRO-3912: Fix Big decimal deser #2599

Conversation

clesaec commented Nov 28, 2023

What is the purpose of the change

Verifying this change

Documentation

Ten0 commented Nov 28, 2023

clesaec commented Nov 28, 2023

Ten0 commented Nov 28, 2023 • edited Loading

martin-g Nov 28, 2023

Choose a reason for hiding this comment

martin-g Nov 28, 2023

Choose a reason for hiding this comment

martin-g Nov 28, 2023

Choose a reason for hiding this comment

martin-g Nov 28, 2023

Choose a reason for hiding this comment

Ten0 Nov 28, 2023

Choose a reason for hiding this comment

martin-g commented Nov 28, 2023

clesaec commented Nov 28, 2023

Ten0 commented Nov 29, 2023 • edited Loading

clesaec commented Nov 29, 2023

Ten0 commented Nov 29, 2023

clesaec commented Nov 29, 2023

martin-g Dec 1, 2023

Choose a reason for hiding this comment

Ten0 commented Dec 1, 2023 • edited Loading

clesaec commented Dec 1, 2023

clesaec commented Dec 4, 2023

Ten0 commented Nov 28, 2023 •

edited

Loading

Ten0 commented Nov 29, 2023 •

edited

Loading

Ten0 commented Dec 1, 2023 •

edited

Loading