You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "amogh-jahagirdar (via GitHub)" <gi...@apache.org> on 2024/07/19 20:11:45 UTC
[PR] Core: Add DataFiles builder API to enable users to specify their own custom conversion logic for string partition values [iceberg]
amogh-jahagirdar opened a new pull request, #10724:
URL: https://github.com/apache/iceberg/pull/10724
This change adds the following API to DataFile builder:
```
withPartitionPath(String newPartitionPath, BiFunction<String, Type, Object> partitionPathConverter)
```
This is done to enable users that want to use the DataFiles builder API with their own custom partition string conversion logic, that way the burden isn't in iceberg itself to handle all various interpretations of string partition values.
Currently, for the `withPartitionPath` API there is default conversion logic from String partition values to objects to be used for Iceberg partition values in [Conversions](https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/types/Conversions.java#L42) for use cases like Hive table migrations.
We probably shouldn't be adding to that Conversions class because Iceberg does not outline how Hive identity string partition values should convert into Iceberg partition values. IMO this is rightfully so, since the string conversion is subject to a lot of interpretation and there can be all sorts of partition strings out there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add DataFiles builder API to enable users to specify their own custom conversion logic for string partition values [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10724:
URL: https://github.com/apache/iceberg/pull/10724#issuecomment-2466519713
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add DataFiles builder API to enable users to specify their own custom conversion logic for string partition values [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #10724: Core: Add DataFiles builder API to enable users to specify their own custom conversion logic for string partition values
URL: https://github.com/apache/iceberg/pull/10724
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add DataFiles builder API to enable users to specify their own custom conversion logic for string partition values [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10724:
URL: https://github.com/apache/iceberg/pull/10724#discussion_r1686880606
##########
core/src/main/java/org/apache/iceberg/DataFiles.java:
##########
@@ -259,11 +268,19 @@ public Builder withFileSizeInBytes(long newFileSizeInBytes) {
}
public Builder withPartitionPath(String newPartitionPath) {
+ return withPartitionPath(newPartitionPath, DEFAULT_PARTITION_STRING_CONVERSION);
+ }
+
+ public Builder withPartitionPath(
+ String newPartitionPath, BiFunction<String, Type, Object> partitionPathConverter) {
Review Comment:
Yes, let me update the description to be clear. But In short, one use case is for Delta Uniform Iceberg metadata conversion. Currently, timestamp types are not supported for partition columns and Iceberg's `Conversion` utility https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/types/Conversions.java#L69 does not handle timestamps.
I was a bit hestitant to add another case for Timestamp to that utility since Iceberg does not specify (and arguably *should not*) specify how to convert string partition values into a given type. For now it's all Hive style assumptions which was done to facilitate migration use cases.
Since Iceberg does not specify this conversion behavior, exposing an API which allows people to pass in their own converter was the first approach that came to mind for solving the problem I had but arguably if that's the only use case, one could also make the case that Delta Uniform should take the responsibility of converting to Partition tuples and using the existing DataFile APIs? Def open to that as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add DataFiles builder API to enable users to specify their own custom conversion logic for string partition values [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10724:
URL: https://github.com/apache/iceberg/pull/10724#discussion_r1684933556
##########
core/src/main/java/org/apache/iceberg/DataFiles.java:
##########
@@ -259,11 +268,19 @@ public Builder withFileSizeInBytes(long newFileSizeInBytes) {
}
public Builder withPartitionPath(String newPartitionPath) {
+ return withPartitionPath(newPartitionPath, DEFAULT_PARTITION_STRING_CONVERSION);
+ }
+
+ public Builder withPartitionPath(
+ String newPartitionPath, BiFunction<String, Type, Object> partitionPathConverter) {
Review Comment:
If there's general support for this idea, I'll add a unit test that exercises a simple custom conversion logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add DataFiles builder API to enable users to specify their own custom conversion logic for string partition values [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10724:
URL: https://github.com/apache/iceberg/pull/10724#discussion_r1686880606
##########
core/src/main/java/org/apache/iceberg/DataFiles.java:
##########
@@ -259,11 +268,19 @@ public Builder withFileSizeInBytes(long newFileSizeInBytes) {
}
public Builder withPartitionPath(String newPartitionPath) {
+ return withPartitionPath(newPartitionPath, DEFAULT_PARTITION_STRING_CONVERSION);
+ }
+
+ public Builder withPartitionPath(
+ String newPartitionPath, BiFunction<String, Type, Object> partitionPathConverter) {
Review Comment:
Yes, let me update the description to be clear. But In short, one use case is for Delta Uniform Iceberg metadata conversion. Currently, timestamp types are not supported and Iceberg's `Conversion` utility https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/types/Conversions.java#L69 does not handle timestamps.
I was a bit hestitant to add another case for Timestamp to that utility since Iceberg does not specify (and arguably *should not*) specify how to convert string partition values into a given type. For now it's all Hive style assumptions which was done to facilitate migration use cases.
Since Iceberg does not specify this conversion behavior exposing an API which allows people to pass in their own converter was the first approach that came to mind for solving the problem I had but arguably if that's the only use case, one could also make the case that Delta Uniform should take the responsibility of converting to Partition tuples and using the existing DataFile APIs? Def open to that as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add DataFiles builder API to enable users to specify their own custom conversion logic for string partition values [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10724:
URL: https://github.com/apache/iceberg/pull/10724#issuecomment-2480862570
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add DataFiles builder API to enable users to specify their own custom conversion logic for string partition values [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #10724:
URL: https://github.com/apache/iceberg/pull/10724#issuecomment-2261582583
I just talked offline with @amogh-jahagirdar about a better way to do this. Systems that store information in strings should have ways to parse those strings to recover the data, and I think that it is important to use those parsing utilities rather than trying to maintain code in Iceberg that mimics their behavior. The right solution is to use the parsing utility from Delta and wrap the values in `PartitionData` to pass into the builder for `DataFile` metadata.
In general, Iceberg has always considered partition strings to be a one-way transform. We produce the strings, but don't attempt to make values recoverable. The parsing code here is extremely simple for Hive table conversions and should not generally be used -- a much better pattern is the one I outlined above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add DataFiles builder API to enable users to specify their own custom conversion logic for string partition values [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10724:
URL: https://github.com/apache/iceberg/pull/10724#discussion_r1688404591
##########
core/src/main/java/org/apache/iceberg/DataFiles.java:
##########
@@ -259,11 +268,19 @@ public Builder withFileSizeInBytes(long newFileSizeInBytes) {
}
public Builder withPartitionPath(String newPartitionPath) {
+ return withPartitionPath(newPartitionPath, DEFAULT_PARTITION_STRING_CONVERSION);
+ }
+
+ public Builder withPartitionPath(
+ String newPartitionPath, BiFunction<String, Type, Object> partitionPathConverter) {
Review Comment:
I like both of those alternatives.
1. Is probably the right solution (our string support is there pretty much expressly for hive) but I have no problem with doing 2. since we don't really have any other use case for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add DataFiles builder API to enable users to specify their own custom conversion logic for string partition values [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10724:
URL: https://github.com/apache/iceberg/pull/10724#discussion_r1687196547
##########
core/src/main/java/org/apache/iceberg/DataFiles.java:
##########
@@ -259,11 +268,19 @@ public Builder withFileSizeInBytes(long newFileSizeInBytes) {
}
public Builder withPartitionPath(String newPartitionPath) {
+ return withPartitionPath(newPartitionPath, DEFAULT_PARTITION_STRING_CONVERSION);
+ }
+
+ public Builder withPartitionPath(
+ String newPartitionPath, BiFunction<String, Type, Object> partitionPathConverter) {
Review Comment:
Possible Alternatives:
1. No new APIs, callers (in this case Uniform) should take the responsibiity of computing partition tuples themselves and use the `withPartition(StructLike partition)` API.
2. Add the conversion logic for timestamps to Conversion. I wanted to avoid this mostly since Iceberg doesn't specify how to convert strings to partition values so whatever would be implemented would have some set of assumptions (iso8601 string with some assumed precision etc), and I didn't want to set a precedent that there is some established conversion behavior that should be followed (since there isn't). But there's a counter argument to that which is, really the only remaining type that's missing in that logic is Timestamps anyways.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add DataFiles builder API to enable users to specify their own custom conversion logic for string partition values [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10724:
URL: https://github.com/apache/iceberg/pull/10724#discussion_r1686880606
##########
core/src/main/java/org/apache/iceberg/DataFiles.java:
##########
@@ -259,11 +268,19 @@ public Builder withFileSizeInBytes(long newFileSizeInBytes) {
}
public Builder withPartitionPath(String newPartitionPath) {
+ return withPartitionPath(newPartitionPath, DEFAULT_PARTITION_STRING_CONVERSION);
+ }
+
+ public Builder withPartitionPath(
+ String newPartitionPath, BiFunction<String, Type, Object> partitionPathConverter) {
Review Comment:
Yes, let me update the description to be clear. But In short, one use case is for Delta Uniform Iceberg metadata conversion. Currently, timestamp types are not supported for partition columns and Iceberg's `Conversion` utility https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/types/Conversions.java#L69 does not handle timestamps.
I was a bit hestitant to add another case for Timestamp to that utility since Iceberg does not specify (and arguably *should not*) specify how to convert string partition values into a given type. For now it's all Hive style assumptions which was done to facilitate migration use cases.
Since Iceberg does not specify this conversion behavior exposing an API which allows people to pass in their own converter was the first approach that came to mind for solving the problem I had but arguably if that's the only use case, one could also make the case that Delta Uniform should take the responsibility of converting to Partition tuples and using the existing DataFile APIs? Def open to that as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add DataFiles builder API to enable users to specify their own custom conversion logic for string partition values [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10724:
URL: https://github.com/apache/iceberg/pull/10724#discussion_r1686839951
##########
core/src/main/java/org/apache/iceberg/DataFiles.java:
##########
@@ -259,11 +268,19 @@ public Builder withFileSizeInBytes(long newFileSizeInBytes) {
}
public Builder withPartitionPath(String newPartitionPath) {
+ return withPartitionPath(newPartitionPath, DEFAULT_PARTITION_STRING_CONVERSION);
+ }
+
+ public Builder withPartitionPath(
+ String newPartitionPath, BiFunction<String, Type, Object> partitionPathConverter) {
Review Comment:
Any system's we are thinking about here? I know we are pretty much good for Hive now, do we have custom apps we are trying to support?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org