You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "clesaec (via GitHub)" <gi...@apache.org> on 2023/11/28 09:43:46 UTC
[PR] AVRO-3912: Fix Big decimal deser [avro]
clesaec opened a new pull request, #2599:
URL: https://github.com/apache/avro/pull/2599
<!--
*Thank you very much for contributing to Apache Avro - we are happy that you want to help us improve Avro. To help the community review your contribution in the best possible way, please go through the checklist below, which will get the contribution into a shape in which it can be best reviewed.*
*Please understand that we do not do this to make contributions to Avro a hassle. In order to uphold a high standard of quality for code contributions, while at the same time managing a large number of contributions, we need contributors to prepare the contributions well, and give reviewers enough contextual information for the review. Please also understand that contributions that do not follow this guide will take longer to review and thus typically be picked up with lower priority by the community.*
## Contribution Checklist
- Make sure that the pull request corresponds to a [JIRA issue](https://issues.apache.org/jira/projects/AVRO/issues). Exceptions are made for typos in JavaDoc or documentation files, which need no JIRA issue.
- Name the pull request in the form "AVRO-XXXX: [component] Title of the pull request", where *AVRO-XXXX* should be replaced by the actual issue number.
The *component* is optional, but can help identify the correct reviewers faster: either the language ("java", "python") or subsystem such as "build" or "doc" are good candidates.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Make sure that the change passes the automated tests. You can [build the entire project](https://github.com/apache/avro/blob/main/BUILD.md) or just the [language-specific SDK](https://avro.apache.org/project/how-to-contribute/#unit-tests).
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message (including the JIRA id)
- Every commit message references Jira issues in their subject lines. In addition, commits follow the guidelines from [How to write a good git commit message](https://chris.beams.io/posts/git-commit/)
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
-->
## What is the purpose of the change
[AVRO-3912](https://issues.apache.org/jira/browse/AVRO-3912) : fix big decimal deserialisation error (thanks @Ten0 for comment on this [PR](https://github.com/apache/avro/pull/2302)).
## 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)
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "Ten0 (via GitHub)" <gi...@apache.org>.
Ten0 commented on PR #2599:
URL: https://github.com/apache/avro/pull/2599#issuecomment-1831118053
By the way if you would put the scale 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. Maybe it isn't too late to change that since it's not released?
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "Ten0 (via GitHub)" <gi...@apache.org>.
Ten0 commented on PR #2599:
URL: https://github.com/apache/avro/pull/2599#issuecomment-1829524081
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, which it looks like it's incorrect because it's only a logical type and not a full new type.
Also IIUC the Java implementation starts by deserializing as bytes and then converts that into a BigDecimal, so we do have the two lengths.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #2599:
URL: https://github.com/apache/avro/pull/2599#issuecomment-1829848963
Let's add an interop test (or extend the current one) for BigDecimal in Rust and Java!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec commented on PR #2599:
URL: https://github.com/apache/avro/pull/2599#issuecomment-1829515346
@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.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #2599:
URL: https://github.com/apache/avro/pull/2599#discussion_r1412062924
##########
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
Review Comment:
`global size` sounds too ... global.
How about `total size` ?
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "Ten0 (via GitHub)" <gi...@apache.org>.
Ten0 commented on PR #2599:
URL: https://github.com/apache/avro/pull/2599#issuecomment-1832050501
> Java decoder interface miss a function like "readRemainder()" or something similar
Isn't `.remaining()` it?
https://github.com/apache/avro/blob/b4968f70f1cb6f220d87063c477bbc6309341c69/lang/java/avro/src/main/java/org/apache/avro/Conversions.java#L90-L93
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec commented on PR #2599:
URL: https://github.com/apache/avro/pull/2599#issuecomment-1832133150
.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.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "Ten0 (via GitHub)" <gi...@apache.org>.
Ten0 commented on code in PR #2599:
URL: https://github.com/apache/avro/pull/2599#discussion_r1407770767
##########
lang/rust/avro/src/decimal.rs:
##########
@@ -214,4 +218,63 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn other() -> TestResult {
+ let schema_str = r#"
+ {
+ "type": "record",
+ "name": "test",
+ "fields": [
+ {
+ "name": "field_name",
+ "type": "bytes",
+ "logicalType": "big-decimal"
+ }
+ ]
+ }
+ "#;
+ let schema = Schema::parse_str(schema_str)?;
+
+ // build record with big decimal value
+ let mut record = Record::new(&schema).unwrap();
+ let val = BigDecimal::new(BigInt::from(12), 2);
+ record.put("field_name", val.clone());
+
+ // save record in writer
+ let codec = Codec::Null;
+ let mut writer = Writer::builder()
+ .schema(&schema)
+ .codec(codec)
+ .writer(Vec::new())
+ .build();
+
+ let result = writer.append(record.clone());
+ assert!(result.is_ok(), "Append KO : {}", result.unwrap_err());
+ assert!(writer.flush().is_ok(), "Flush KO");
Review Comment:
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
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec commented on PR #2599:
URL: https://github.com/apache/avro/pull/2599#issuecomment-1838013063
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
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "Ten0 (via GitHub)" <gi...@apache.org>.
Ten0 commented on PR #2599:
URL: https://github.com/apache/avro/pull/2599#issuecomment-1836241379
> .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.
`ByteBuffer` is exactly what you have when decoding `big-decimal`.
https://github.com/apache/avro/blob/b4968f70f1cb6f220d87063c477bbc6309341c69/lang/java/avro/src/main/java/org/apache/avro/Conversions.java#L171-L188
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec merged PR #2599:
URL: https://github.com/apache/avro/pull/2599
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "Ten0 (via GitHub)" <gi...@apache.org>.
Ten0 commented on PR #2599:
URL: https://github.com/apache/avro/pull/2599#issuecomment-1829509467
I thought the error was on serialization, not deserialization
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #2599:
URL: https://github.com/apache/avro/pull/2599#discussion_r1407749504
##########
lang/rust/avro/src/decimal.rs:
##########
@@ -214,4 +218,63 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn other() -> TestResult {
Review Comment:
Please give the method a better name, e.g. `test_avro_3912_...`
##########
lang/rust/avro/src/decimal.rs:
##########
@@ -214,4 +218,63 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn other() -> TestResult {
+ let schema_str = r#"
+ {
+ "type": "record",
+ "name": "test",
+ "fields": [
+ {
+ "name": "field_name",
+ "type": "bytes",
+ "logicalType": "big-decimal"
+ }
+ ]
+ }
+ "#;
+ let schema = Schema::parse_str(schema_str)?;
+
+ // build record with big decimal value
+ let mut record = Record::new(&schema).unwrap();
+ let val = BigDecimal::new(BigInt::from(12), 2);
+ record.put("field_name", val.clone());
+
+ // save record in writer
+ let codec = Codec::Null;
+ let mut writer = Writer::builder()
+ .schema(&schema)
+ .codec(codec)
+ .writer(Vec::new())
+ .build();
+
+ let result = writer.append(record.clone());
+ assert!(result.is_ok(), "Append KO : {}", result.unwrap_err());
Review Comment:
```suggestion
assert!(result.is_ok(), "Append NOK : {result}");
```
##########
lang/rust/avro/src/decimal.rs:
##########
@@ -214,4 +218,63 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn other() -> TestResult {
+ let schema_str = r#"
+ {
+ "type": "record",
+ "name": "test",
+ "fields": [
+ {
+ "name": "field_name",
+ "type": "bytes",
+ "logicalType": "big-decimal"
+ }
+ ]
+ }
+ "#;
+ let schema = Schema::parse_str(schema_str)?;
+
+ // build record with big decimal value
+ let mut record = Record::new(&schema).unwrap();
+ let val = BigDecimal::new(BigInt::from(12), 2);
+ record.put("field_name", val.clone());
+
+ // save record in writer
Review Comment:
```suggestion
// write a record
```
##########
lang/rust/avro/src/decimal.rs:
##########
@@ -214,4 +218,63 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn other() -> TestResult {
+ let schema_str = r#"
+ {
+ "type": "record",
+ "name": "test",
+ "fields": [
+ {
+ "name": "field_name",
+ "type": "bytes",
+ "logicalType": "big-decimal"
+ }
+ ]
+ }
+ "#;
+ let schema = Schema::parse_str(schema_str)?;
+
+ // build record with big decimal value
+ let mut record = Record::new(&schema).unwrap();
+ let val = BigDecimal::new(BigInt::from(12), 2);
+ record.put("field_name", val.clone());
+
+ // save record in writer
+ let codec = Codec::Null;
+ let mut writer = Writer::builder()
+ .schema(&schema)
+ .codec(codec)
+ .writer(Vec::new())
+ .build();
+
+ let result = writer.append(record.clone());
+ assert!(result.is_ok(), "Append KO : {}", result.unwrap_err());
+ assert!(writer.flush().is_ok(), "Flush KO");
Review Comment:
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.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec commented on PR #2599:
URL: https://github.com/apache/avro/pull/2599#issuecomment-1829971763
Get it, i didn't realize that a logical type has to be deserializable in its main type; i do the fix.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec commented on PR #2599:
URL: https://github.com/apache/avro/pull/2599#issuecomment-1832001680
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).
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3912: Fix Big decimal deser [avro]
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec commented on PR #2599:
URL: https://github.com/apache/avro/pull/2599#issuecomment-1836308579
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 ?
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org