Repository navigation
Add generalized support for Apache Compress formats - #7497
Conversation
…#2741 - Add test for Unix compress .Z files (supported by base Apache Compress) - add optional dependency XZ to enable support for XZ, LZMA, & 7zip - SevenZip decompressor requires special handling since it uses a SeekableByteChannel rather than a Stream - Add xz to the list of compressors that support concatenated streams
github-actions
Bot
added
Type: Feature Request
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive support for additional Apache Compress formats including XZ, LZMA, 7zip, ZStandard, and Unix compress (.Z files). The changes enable OpenRefine to handle these compression formats during import operations with proper streaming support where available and special handling for SevenZip's seekable channel requirements.
Key Changes:
- Added XZ and ZStandard dependencies to enable new compression format support
- Implemented special handling for 7zip archives using SeekableByteChannel
- Extended test coverage for all newly supported compression formats (.Z, .lzma, .xz, .zst, .7z)
Reviewed Changes
Copilot reviewed 5 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/core/pom.xml | Added xz and zstd-jni dependencies to support new compression formats |
| modules/core/src/main/java/com/google/refine/importing/ImportingUtilities.java | Added SevenZFile handling, StreamingNotSupportedException catch, and refactored stream management |
| modules/core/src/main/java/com/google/refine/importing/DefaultImportingController.java | Added TODO comments for optimizing preview data processing |
| main/tests/server/src/com/google/refine/importing/ImportingUtilitiesTests.java | Extended tests to cover new formats and refactored archive import tests to be format-agnostic |
| main/tests/data/persons.csv.Z | Added test file for Unix compress format |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 9 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
SoryRawyer
left a comment
SoryRawyer
left a comment
There was a problem hiding this comment.
I just have the one question about the buffer size variable but otherwise this looks good to me.
- remove unused constant - remove redundant/ignored keywords - remove redundant semicolons
- Now correctly handles .tar.gz / .tgz files - Add a test case to cover this so it doesn't break again
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
modules/core/src/main/java/com/google/refine/importing/ImportingUtilities.java:1
- Corrected 'it's' to 'its' - should use possessive form rather than contraction.
/*
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@SoryRawyer Sorry to ask you to review this twice. I removed the constant mentioned in your previous review, but also, more importantly, fixed the import of compressed archives (e.g. .tar.gz) and added a test to cover that case. |
SoryRawyer
left a comment
SoryRawyer
left a comment
There was a problem hiding this comment.
I just have that one small question, but that shouldn't block anything. Looks good and I think it's easier to read now too.
| } | ||
| if (!entry.isDirectory()) { | ||
| ObjectNode fileRecord2 = processArchiveEntry(rawDataDir, archiveFileRecord, progress, entry.getName(), | ||
| zf.getInputStream(entry)); |
There was a problem hiding this comment.
Should this be in a try-with-resources block like the 7zip case below, or does that not make much of a difference here?
There was a problem hiding this comment.
Good catch. For some reason my IDE didn't flag either of these cases, which I don't really understand, but the second one is fixed now too.
Fixes #2741
Changes proposed in this pull request:
a SeekableByteChannel rather than a Stream