misc: Make WriterOptions serializable#14868
misc: Make WriterOptions serializable#14868yingsu00 wants to merge 1 commit intofacebookincubator:mainfrom
Conversation
meta-cla
bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
@yingsu00 Do you want to upgrade this PR from 'draft'?
- CI is red. Please, take a look.
- Use "feat: " prefix for PR title.
- Please, add PR description to clarify the purpose of this change (a stepping stone towards making HiveInsertTableHandle serializable)
| folly::dynamic obj = folly::dynamic::object; | ||
| obj["name"] = "WriterOptions"; | ||
|
|
||
| // 1) Schema |
There was a problem hiding this comment.
Ordinal numbers like this are fragile as they do not survice refactoring. Please, drop.
Also, these comments seem redundant as they just repeat the code. Consider dropping altogether.
| // 5) adjustTimestampToTimezone | ||
| obj["adjustTimestampToTimezone"] = adjustTimestampToTimezone; | ||
|
|
||
| // We do *not* serialize pool, spillConfig, nonReclaimableSection, |
There was a problem hiding this comment.
Would you move this comment to the header file? I assume the caller of 'create' would need to be aware of these.
| } | ||
|
|
||
| if (auto p = obj.get_ptr("serdeParameters")) { | ||
| opts->serdeParameters.clear(); |
|
|
||
| if (auto p = obj.get_ptr("serdeParameters")) { | ||
| opts->serdeParameters.clear(); | ||
| for (auto& kv : p->items()) { |
There was a problem hiding this comment.
for (const auto& [key, value] : p->items())
| opts->adjustTimestampToTimezone = p->asBool(); | ||
| } | ||
|
|
||
| // TODO: Finish spillConfig. We currently do not serialize it. |
There was a problem hiding this comment.
TODO: Finish spillConfig.
What does that mean? Would you elaborate? Can you address this TODO now?
| */ | ||
| #include <gtest/gtest.h> | ||
|
|
||
| // #include "velox/common/compression/Compression.h" |
|
|
||
| auto opts = std::make_shared<WriterOptions>(); | ||
|
|
||
| // Schema: row<a:bigint> |
There was a problem hiding this comment.
Drop redundant comments.
| // Basic shape checks on serialized output | ||
| ASSERT_TRUE(serialized.isObject()); | ||
| // Always present: | ||
| ASSERT_TRUE(serialized.count("adjustTimestampToTimezone") == 1); |
There was a problem hiding this comment.
ASSERT_EQ
However, let's not hard-code serde format in the test. Rather, let's create round-trip test.
| EXPECT_EQ(serialized.count("flushPolicyFactory"), 0); | ||
|
|
||
| // Deserialize | ||
| auto roundTripped = ISerializable::deserialize<WriterOptions>(serialized); |
| }; | ||
|
|
||
| struct WriterOptions { | ||
| struct WriterOptions : public ISerializable { |
There was a problem hiding this comment.
I see that WriterOptions has a virtual method (processConfigs), which means that it has derived classes, which may have additional state. Serde needs to take this into account.
|
@mbasmanova Thank you very much for reviewing this PR. I'll address your comments tomorrow. |
PingLiuPing
left a comment
PingLiuPing
left a comment
There was a problem hiding this comment.
Should we need to serialize the sub class too? Such as velox::parquet::WriterOptions and velox::dwrf::WriterOptions.
|
This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions! |
3 participants
No description provided.