-
Notifications
You must be signed in to change notification settings - Fork 301
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
HPCC-29385 Allow incremental decompression from lz4 files #17557
Conversation
https://track.hpccsystems.com/browse/HPCC-29385 |
system/jlib/jfcmp.hpp
Outdated
@@ -20,6 +20,7 @@ | |||
|
|||
#include "platform.h" | |||
#include "jlzw.hpp" | |||
#include "jlzw.ipp" |
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.
minor: I don't like ipp's but looks odd that a hpp is including an ipp, could we move CFcmpExpander into the ipp and have that included where needed instead here?
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 agree, I'll refactor to avoid that.
system/jlib/jlzw.cpp
Outdated
//Preallocate the expansion target to the block size - to ensure it is the right size and | ||
//avoid reallocation when expanding lz4 | ||
curblockbuf.reserve(trailer.blockSize); | ||
curblockbuf.clear(); |
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.
should the abobe 2 lines be ensureCapacity ?
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.
blockSize is the compressed block size, but curblockbuf is used to expand into, so it won't be big enough.
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.
It will be enough because LZ4 only compresses at most the block size into a single chunk.
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.
Yes, that would be better - I couldn't find that function...despite looking.
system/jlib/jlzw.cpp
Outdated
//Start decompressing again and avoid re-reading the data from disk | ||
void * rawData; | ||
if (fileio) | ||
rawData = compressedInputBlock.mem(); |
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.
could/should use a const void * here , and compressedInputBlock.get() ?
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.
Yes, I will change
https://track.hpccsystems.com/browse/HPCC-29385 |
system/jlib/jlz4.cpp
Outdated
@@ -221,6 +222,66 @@ class jlib_decl CLZ4Expander : public CFcmpExpander | |||
} | |||
} | |||
|
|||
size32_t expandFirst(MemoryBuffer & target, const void * src) |
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.
this and expandNext should have 'virtual' and 'override'
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.
@ghalliday - 1 new trivial comment
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.
@ghalliday - looks good.
Signed-off-by: Gavin Halliday <[email protected]>
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.
Looks good.
Type of change:
Checklist:
Smoketest:
Testing: