Skip to content

Conversation

noxthot
Copy link
Contributor

@noxthot noxthot commented Jun 3, 2025

Fixes #189 and #191 (both tested locally)

Copy link

codecov bot commented Jun 3, 2025

Codecov Report

Attention: Patch coverage is 10.49383% with 145 lines in your changes missing coverage. Please review.

Project coverage is 62.10%. Comparing base (3072e71) to head (e26ce81).

Files with missing lines Patch % Lines
src/PAR2/PAR2_types.jl 10.49% 145 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #192       +/-   ##
===========================================
- Coverage   86.38%   62.10%   -24.29%     
===========================================
  Files          10       11        +1     
  Lines        1631     2813     +1182     
===========================================
+ Hits         1409     1747      +338     
- Misses        222     1066      +844     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sandyspiers
Copy link

Thanks for this! Fixes the issue for me as well. Would love to see this merged :)

@nhz2
Copy link
Member

nhz2 commented Aug 21, 2025

Thank you for working on this.
I think there needs to be more automated tests added and a review from someone familiar with the added fetures before this can be merged.

@noxthot
Copy link
Contributor Author

noxthot commented Aug 21, 2025

Thanks for looking into this. While I understand the need for more tests, I want to note that the previous thrift-file updates pull requests also have been merged without further tests:

The thrift file comes from the Apache project, the jl files are autogenerated by Thrift.jl. Probably it would be nice to have an automated workflow (similar to Dependabot) which looks for new thrift-files and autogenerates a pull-request using a fixed recipe to minimize failure and security risks.

@nhz2
Copy link
Member

nhz2 commented Aug 21, 2025

We want to avoid a situation like https://gitlab.com/ExpandingMan/Parquet2.jl/-/merge_requests/32#503d0b7f8cff0d2c6b891b801fa66e740318fbff

Also, this PR goes straight from 2.8.0 to 2.11.0, it would probably be better to carefully go through the format versions updates one at a time, starting with https://parquet.apache.org/blog/2021/10/06/2.9.0/

@noxthot
Copy link
Contributor Author

noxthot commented Aug 21, 2025

You are absolutely right that such situations should be avoided.

At the moment, I do not have the capacity to delve into this. If anyone else is able to invest some time, please feel free to proceed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"KeyError: key 7 not found" when loading parquet files generated with parquet and arrow crates
3 participants