fix(compression): Match compressed-file suffixes case-insensitively (fixes #2414). - #2419
fix(compression): Match compressed-file suffixes case-insensitively (fixes #2414).#2419junhaoliao wants to merge 10 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughCompressed archive size estimation now matches supported suffixes case-insensitively in Python and Rust. Zstandard detection is limited to ChangesCompressed-size estimation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/compression-coordinator/src/partition.rs`:
- Around line 413-425: Expand the Zstandard suffix tests in
components/compression-coordinator/src/partition.rs lines 413-425 and
integration-tests/tests/test_clp_native_py_project_imports.py lines 28-38. Add
.zstd, .zstandard, .tar.zstd, and .tar.zstandard cases to both suites, asserting
estimate_uncompressed_size or estimated_uncompressed_size returns the original
file size rather than applying decompression expansion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aa7e7440-002e-4679-b642-8a4b162d87d3
📒 Files selected for processing (3)
components/clp-py-utils/clp_py_utils/core.pycomponents/compression-coordinator/src/partition.rsintegration-tests/tests/test_clp_native_py_project_imports.py
| #[test] | ||
| fn test_estimate_uncompressed_size_for_zstandard_suffix() { | ||
| const FILE_SIZE: u64 = 100; | ||
|
|
||
| for key in [ | ||
| "logs/app.log.zst", | ||
| "logs/app.log.clp.zst", | ||
| "logs/app.log.tar.zst", | ||
| "logs/app.log.ZST", | ||
| ] { | ||
| assert_eq!(FILE_SIZE * 8, estimate_uncompressed_size(key, FILE_SIZE)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the full Zstandard suffix contract in both test suites.
Both suites verify supported .zst variants but omit the suffixes that must no longer be recognised:
components/compression-coordinator/src/partition.rs#L413-L425: assert.zstd,.zstandard,.tar.zstd, and.tar.zstandardreturnFILE_SIZE.integration-tests/tests/test_clp_native_py_project_imports.py#L28-L38: add the same negative cases and assertestimated_uncompressed_size == file_size.
📍 Affects 2 files
components/compression-coordinator/src/partition.rs#L413-L425(this comment)integration-tests/tests/test_clp_native_py_project_imports.py#L28-L38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/compression-coordinator/src/partition.rs` around lines 413 - 425,
Expand the Zstandard suffix tests in
components/compression-coordinator/src/partition.rs lines 413-425 and
integration-tests/tests/test_clp_native_py_project_imports.py lines 28-38. Add
.zstd, .zstandard, .tar.zstd, and .tar.zstandard cases to both suites, asserting
estimate_uncompressed_size or estimated_uncompressed_size returns the original
file size rather than applying decompression expansion.
There was a problem hiding this comment.
omit the suffixes that must no longer be recognised
i'm not sure if that's a must. seems like adding such tests would also only add maintenance debt rather than actual value. what do you think?
There was a problem hiding this comment.
@junhaoliao, I think the negative cases are justified here, though I agree they should not be added merely for exhaustive extension testing.
They protect the main behavioral boundary of #2414: these suffixes were previously accepted and are now intentionally unsupported. The existing positive tests prove that .zst still works, but would not catch a later broadening back to .zstd/.zstandard—for example, if someone replaces the precise .zst check with a more permissive “zstd-like” matcher.
Since the four cases directly correspond to the removed aliases (including their .tar forms), the maintenance cost is small and the regression value is concrete. I’d keep the cases, but they can be compactly expressed as a single loop in each suite.
🐇💭
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/compression-coordinator/src/partition.rs`:
- Around line 241-249: Update estimate_uncompressed_size to remove the
per-object to_ascii_lowercase allocation. Add an allocation-free
ends_with_ignore_ascii_case helper using the existing suffix-length and
substring approach, then use it for each gzip suffix and the .zst check while
preserving the current compression estimates and fallback behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cc056d3f-5b68-41ff-a6f8-8766986a118f
📒 Files selected for processing (2)
components/clp-py-utils/clp_py_utils/core.pycomponents/compression-coordinator/src/partition.rs
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
The issue reported in #2414 is to restrict the suffix for zstdandard to .zst. It doesn't mention case sensitivity.
@junhaoliao Why do we need case-insensitive match?
| /// | ||
| /// The estimated uncompressed size. | ||
| fn estimate_uncompressed_size(key: &str, size: u64) -> u64 { | ||
| fn estimate_uncompressed_size(path: &str, size: u64) -> u64 { |
There was a problem hiding this comment.
I'm not sure if using path is a good option here since the path is bucket + key.
| .any(|suffix| ends_with_ignore_ascii_case(key, suffix)) | ||
| { | ||
| let path = path.to_ascii_lowercase(); | ||
| if GZIP_SUFFIXES.iter().any(|suffix| path.ends_with(suffix)) { |
There was a problem hiding this comment.
Please explain why you need let path = path.to_ascii_lowercase();.
Similar questions asked by 3c66a96#r3684813671.
Bill-hbrhbr
left a comment
Bill-hbrhbr
left a comment
There was a problem hiding this comment.
I think the lowercase feature is useful. It is not tracked in issue 2414 but I suppose we can update the PR title to better reflect what's being fixed and what's being added.
|
For title, how about: |
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
4 participants
Description
The Rust compression coordinator and Python
FileMetadataestimated Zstandard files using non-standard.zstdand.zstandardsuffixes while overlooking the standard.zstsuffix.This change updates both implementations to recognize the standard
.zstextension case-insensitively and applies the same case-insensitive matching to the existing gzip suffixes. The Rust implementation uses a shared allocation-free suffix helper and does not suppress Clippy checks. Focused regression tests cover.zst,.clp.zst,.tar.zst, uppercase.ZST, and lower- and uppercase forms of every supported gzip suffix.Checklist
breaking change.
Validation performed
task lint:fix-py(passed; the task reported existing non-blocking mypy diagnostics in unrelated Python components).task lint:fix-rust(passed).task tests:integration:clp-py-project-imports:task tests:rust-all:task:./sbin/start-clp.shfailed while starting the filesystem-input package:./sbin/stop-clp.shthen removed the partially started stack successfully (stop_status=0).Summary by CodeRabbit
Bug Fixes
.gz,.gzip,.tgz, and.tar.gzreceive accurate uncompressed-size estimates..zst, including nested archive names.Tests