-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Ion Scan #544
Add Ion Scan #544
Conversation
Conformance comparison report
Number passing in both: 5602 Number failing in both: 826 Number passing in Base (b970752) but now fail: 0 Number failing in Base (b970752) but now pass: 1 The following test(s) were previously failing but now pass. Before merging, confirm they are intended to pass: Click here to see
|
95a28d7
to
68f01d4
Compare
8aaecbf
to
40f679b
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (61.53%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #544 +/- ##
==========================================
- Coverage 80.12% 80.01% -0.11%
==========================================
Files 95 98 +3
Lines 20581 20630 +49
Branches 20581 20630 +49
==========================================
+ Hits 16490 16508 +18
- Misses 3648 3670 +22
- Partials 443 452 +9 ☔ View full report in Codecov by Sentry. |
.map(|elt| Cow::Owned(self.child_value(elt.clone()))), // TODO find a way to remove clone | ||
BoxedIonValue::Value(elt) => match elt.expect_sequence() { | ||
Ok(seq) => seq | ||
.iter() | ||
.nth(k as usize) | ||
.map(|elt| Cow::Owned(self.child_value(elt.clone()))), // TODO remove clone | ||
Err(_) => todo!(), | ||
.map(|elt| Cow::Owned(self.child_value(elt.clone()))), // TODO find a way to remove clone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an easy way to remove this clone in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No
if header.starts_with(&[0x1f, 0x8b]) { | ||
let decoder = flate2::read::GzDecoder::new(reader); | ||
let buffered = BufReader::new(decoder); | ||
parse_ion_buff(buffered) | ||
} else if header.starts_with(&[0x28, 0xB5, 0x2F, 0xFD]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious where do these header vars come from? Could include some comments explaining where they come from. Similarly in read_ion.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add comments to the code.
Cf. https://datatracker.ietf.org/doc/html/rfc1952#page-6 section 2.3.1
Cf. https://datatracker.ietf.org/doc/rfc8478/ section 3.1.1
#[error("Ion Stream Error: `{}`", .0)] | ||
IonStreamError(IonDecodeError), | ||
|
||
/// Ion Stream Error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Ion Stream Error | |
/// Ion Read Error |
40f679b
to
c459c1b
Compare
c459c1b
to
5deb50e
Compare
This PR builds on #540, #542, & #543 and adds an ion_scan that reads boxed ion variants.
There are still
todo
s and some additional test failures that will be addressed by future PRs that add functionality.To see the ultimate end-point of this integration, refer to #536 and note the test coverage and conformance test results.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.