aokolnychyi commented on code in PR #11044:
URL: https://github.com/apache/iceberg/pull/11044#discussion_r1737974697
##########
core/src/main/java/org/apache/iceberg/InheritableMetadataFactory.java:
##########
@@ -46,11 +49,14 @@ static class BaseInheritableMetadata implements
InheritableMetadata {
private final int specId;
private final long snapshotId;
private final long sequenceNumber;
+ private final String manifestPath;
- private BaseInheritableMetadata(int specId, long snapshotId, long
sequenceNumber) {
+ private BaseInheritableMetadata(
+ int specId, long snapshotId, long sequenceNumber, String manifestPath)
{
Review Comment:
I agree. It seems easier than hacking `GenericAvroReader` like we do for
positions in manifests.
##########
api/src/main/java/org/apache/iceberg/ContentFile.java:
##########
@@ -192,4 +192,12 @@ default F copyWithStats(Set<Integer> requestedColumnIds) {
default F copy(boolean withStats) {
return withStats ? copy() : copyWithoutStats();
}
+
+ /**
+ * Returns the path of the manifest which this file is referenced in or null
if it was not read
+ * from a manifest.
+ */
+ default CharSequence manifestPath() {
Review Comment:
We need to discuss using `CharSequence` for paths in our APIs. I am not sure
this approach is working out very well. I believe the idea was to expose Avro
`Utf8` directly. The problem is that `Utf8` is not serializable. Therefore, we
always convert `Utf8` to `String`. Working with `CharSequence` adds a lot of
complexity for no performance benefit. Puffin and a few other places use
`String` for paths.
@rdblue, is my assumption about `CharSequence` correct? What are your
thoughts now?
##########
api/src/main/java/org/apache/iceberg/ContentFile.java:
##########
@@ -192,4 +192,12 @@ default F copyWithStats(Set<Integer> requestedColumnIds) {
default F copy(boolean withStats) {
return withStats ? copy() : copyWithoutStats();
}
+
+ /**
+ * Returns the path of the manifest which this file is referenced in or null
if it was not read
+ * from a manifest.
+ */
+ default CharSequence manifestPath() {
Review Comment:
Shall we put this method before `pos()` so both are co-located?
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
