-
Notifications
You must be signed in to change notification settings - Fork 304
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-30781 Introduce format registry for pluggable file formats #18334
HPCC-30781 Introduce format registry for pluggable file formats #18334
Conversation
https://track.hpccsystems.com/browse/HPCC-30781 |
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.
ParquetDiskRowReader changes look good. Other changes make sense as well.
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.
jlib_decl is the only change that is needed before merging
system/jlib/jfile.hpp
Outdated
@@ -788,4 +788,9 @@ extern jlib_decl IPropertyTreeIterator * getPlanesIterator(const char * category | |||
extern jlib_decl IFileIO *createBlockedIO(IFileIO *base, size32_t blockSize); | |||
extern jlib_decl size32_t getBlockedFileIOSize(const char *planeName, size32_t defaultSize=0); | |||
|
|||
//---- Pluggable file type related functions ---------------------------------------------- | |||
|
|||
void addAvailableGenericFileTypeName(const char * name); |
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.
Needs jlib_decl to ensure the symbol is visible outside the dll/so. (extern also generally added for clarify - see above).
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.
Declarations added.
common/thorhelper/thorread.cpp
Outdated
|
||
MODULE_EXIT() | ||
{ | ||
genericFileTypeMap.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.
trivial - not needed, will be destroyed when dll is unloaded
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.
Remove.
// should be defined here; the key is the lowecase name of the format, | ||
// as will be used in ECL, and the value should be a lambda | ||
// that creates the appropriate disk row reader object | ||
genericFileTypeMap.emplace("flat", [](IDiskReadMapping * _mapping) { return new BinaryDiskRowReader(_mapping); }); |
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.
In the future I suspect this will change to registering factory class instances that have createReader()/writer methods, but this is good for now.
@ghalliday I made some small changes based on your feedback. Please give it a once-over. Thanks! |
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. Please can you squash the commits and I will merge
9014551
to
d343b9e
Compare
@ghalliday Squashed and ready to merge. Thanks! |
The goal of this PR is to define an easy-to-maintain interface between code that supports new file format types and the rest of the platform, at both compile and execution time.
Type of change:
Checklist:
Smoketest:
Testing:
The compiler successfully generates local a.out binaries (hthor format). It does crash when attempting to read Parquet files, as that code does not yet correctly extract the file name/path.