feat: Add Iceberg partition name generation utility #15443
Conversation
✅ Deploy Preview for meta-velox canceled.
|
meta-cla
bot
added
the
CLA Signed
| std::string toPartitionString(int32_t value, const TypePtr& type) | ||
| const override; | ||
|
|
||
| /// Converts a Timestamp partition key to its string representation. |
There was a problem hiding this comment.
Converts a Timestamp partition key to its string representation.
This sentence is redundant. Consider dropping.
|
|
||
| namespace facebook::velox::connector::hive::iceberg { | ||
|
|
||
| constexpr int32_t kEpochYear = 1970; |
There was a problem hiding this comment.
move this into the only function where it is used.
| if (type->isDate()) { | ||
| return DATE()->toString(value); | ||
| } | ||
| return folly::to<std::string>(value); |
| } | ||
| } | ||
|
|
||
| std::string IcebergPartitionPath::toPartitionString( |
There was a problem hiding this comment.
perhaps, add a check for transform type. I assume it must be kIdentity.
There was a problem hiding this comment.
Yes, you are right
| EXPECT_EQ( | ||
| test( | ||
| TransformType::kIdentity, | ||
| StringView("\x48\x65\x6c\x6c\x6f"), |
There was a problem hiding this comment.
consider adding helper method testVarbinary that takes a single std::string argument. This will make code shorter and easier to read and allow testing long strings.
There was a problem hiding this comment.
Thank you for the suggestion.
|
|
||
| TEST(IcebergPartitionPathTest, identityTimestamp) { | ||
| EXPECT_EQ( | ||
| test(TransformType::kIdentity, Timestamp(0, 0), TIMESTAMP()), |
There was a problem hiding this comment.
add testTimestamp helper method that takes a single Timestamp argument
|
|
||
| TEST(IcebergPartitionPathTest, truncateString) { | ||
| EXPECT_EQ( | ||
| test(TransformType::kTruncate, StringView("abc"), VARCHAR()), "abc"); |
There was a problem hiding this comment.
I assume tranform type doesn't matter here. The results would be the same for kIdentity and kTruncate. If so, consider adding a helper method testString(std::string) which converts string using kIdentity and kTruncate, assert that results are the same and returns that result.
There was a problem hiding this comment.
Thanks.
I added a helper function testString.
| namespace { | ||
|
|
||
| template <typename T> | ||
| std::string test(TransformType transform, T value, const TypePtr& type) { |
There was a problem hiding this comment.
Since we are not testing anything here, let's rename to something like toPath.
PingLiuPing
force-pushed
the
lp_iceberg_partition_path
branch
from
219ec2a to
2dee873
Compare
2 participants
Add Iceberg partition path utility for partition path naming.
Introduces
IcebergPartitionPath, a utility class that converts partition keys totheir string representations for use in Iceberg partition directory paths.
The implementation follows the behavior of the Apache Iceberg Java library.