fix(clp-s): Allow reading archives with zero records (fixes #534).#2338
fix(clp-s): Allow reading archives with zero records (fixes #534).#2338gibber9809 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughIn ChangesEmpty Schema Handling in ArchiveReader
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/src/clp_s/ArchiveReader.cpp`:
- Around line 147-149: The header documentation for the
ArchiveReader::read_metadata() method currently states that it throws when
archive metadata is empty, but the implementation now returns success when
num_schemas is zero. Update the header comment for the read_metadata() method to
accurately reflect the new behavior, removing any references to throwing
exceptions on empty schemas and clarifying that zero schemas is a valid success
condition.
- Around line 147-149: In the early return path when num_schemas equals zero,
the m_table_metadata_decompressor is never closed, leaving the decompressor
lifecycle asymmetric. Add a call to m_table_metadata_decompressor.close()
immediately before the early return statement on line 149, after the reader
checkin but before returning success, to ensure the decompressor is properly
closed and does not retain invalid or stale reader state.
🪄 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
Run ID: ff1c2005-998c-47de-a92a-0bf626441b46
📒 Files selected for processing (1)
components/core/src/clp_s/ArchiveReader.cpp
Description
This PR fixes a longstanding issue where clp-s refuses to decompress or search archives containing zero records. The only thing preventing this from working anymore is that
ArchiveReaderconservatively throws an exception whenever there are zero ERTs stored in an archive for fear of the rest of the implementation being broken when there are no ERTs. The rest of the implementation actually works fine when there are no ERTs, so the fix is to simply remove the exception thrown byArchiveReader.Checklist
breaking change.
Validation performed
*returns no results, and fails schema matching$_filename: ...correctly succeeds range index matching (and subsequently fails schema matching) when the name of the empty file is specified*correctly returns the empty object{}{}Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes