feat: Make ReaderOptions serializable#361
Conversation
ZacBlanco
force-pushed
the
readeroptions-serializable
branch
from
82fe12c to
969a6d8
Compare
|
@yingsu00 PTAL when you have a chance! |
| return obj; | ||
| } | ||
|
|
||
| static ReaderOptions deserialize( |
There was a problem hiding this comment.
@ZacBlanco The conventional signature for this function is not called "deserialize" but "create"
std::shared_ptr(or unique_ptr) create(
const folly::dynamic& obj,
void* context)
I don't know why I added it as "deserialize" or someone updated the name for me. But all other deserialization functions within the object classes were called "create()". Only when we call from ISerializable that we call it "deserialize": auto names = ISerializable::deserialize<std::vector<std::string>>(obj["names"]);
There was a problem hiding this comment.
I updated the method name to create for both ReaderOptions and WriterOptions
There was a problem hiding this comment.
I updated the method name to
createfor both ReaderOptions and WriterOptions
I updated the method name to
createfor both ReaderOptions and WriterOptions
Thank you!
ZacBlanco
force-pushed
the
readeroptions-serializable
branch
from
b344142 to
6ece71d
Compare
| // pool, nonReclaimableSection, and spillConfig callbacks/executor remain at | ||
| // default and must be re-injected by the host. | ||
| std::shared_ptr<WriterOptions> WriterOptions::deserialize( | ||
| std::shared_ptr<WriterOptions> WriterOptions::create( |
There was a problem hiding this comment.
It changes the name of the function to create() from deserialize(). Are there any specific reasons?
3 participants
What problem does this PR solve?
Issue Number: One more step towards #250
Type of Change
Description
In order to prepare for more connector refactoring, DWIO ReaderOptions needs to be serializable.
Performance Impact
Release Note
Please describe the changes in this PR
Release Note:
Checklist (For Author)
Breaking Changes