You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2024/11/19 11:53:36 UTC
[PR] API, Core: Add formatVersion() to Table [iceberg]
nastra opened a new pull request, #11587:
URL: https://github.com/apache/iceberg/pull/11587
(no comment)
--
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] API, Core: Add formatVersion() to Table [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11587:
URL: https://github.com/apache/iceberg/pull/11587#discussion_r1848691024
##########
api/src/main/java/org/apache/iceberg/Table.java:
##########
@@ -373,4 +373,14 @@ default Snapshot snapshot(String name) {
return null;
}
+
+ /**
+ * Returns the format version of the table
Review Comment:
nit: you can do a single line for this javadoc
like
```java
/** Returns the type of changes produced by this task (i.e. insert/delete). */
```
--
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] API, Core: Add formatVersion() to Table [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #11587:
URL: https://github.com/apache/iceberg/pull/11587#issuecomment-2502200150
I think I'd prefer #11620 for now, not a strong opinion, though.
--
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] API, Core: Add formatVersion() to Table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra closed pull request #11587: API, Core: Add formatVersion() to Table
URL: https://github.com/apache/iceberg/pull/11587
--
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] API, Core: Add formatVersion() to Table [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11587:
URL: https://github.com/apache/iceberg/pull/11587#discussion_r1849121688
##########
core/src/main/java/org/apache/iceberg/BaseMetadataTable.java:
##########
@@ -212,6 +212,11 @@ public String toString() {
return name();
}
+ @Override
+ public int formatVersion() {
+ return table().formatVersion();
Review Comment:
Yeah I don't think it makes sense for metadata tables to have a format version. The entries in the tables could be spread across different "versions" of a table where the underlying table had different format versions.
--
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] API, Core: Add formatVersion() to Table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11587:
URL: https://github.com/apache/iceberg/pull/11587#discussion_r1849692234
##########
core/src/main/java/org/apache/iceberg/BaseMetadataTable.java:
##########
@@ -212,6 +212,11 @@ public String toString() {
return name();
}
+ @Override
+ public int formatVersion() {
+ return table().formatVersion();
Review Comment:
agreed, I've removed 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] API, Core: Add formatVersion() to Table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11587:
URL: https://github.com/apache/iceberg/pull/11587#discussion_r1849692234
##########
core/src/main/java/org/apache/iceberg/BaseMetadataTable.java:
##########
@@ -212,6 +212,11 @@ public String toString() {
return name();
}
+ @Override
+ public int formatVersion() {
+ return table().formatVersion();
Review Comment:
so the only table where this would be currently necessary to have is `PositionDeletesTable`. This is so that we can know in `SparkPositionDeletesRewrite` (which runs against `PositionDeletesTable`) whether existing position deletes need to be rewritten to either V2 positional deletes or to DVs
--
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] API, Core: Add formatVersion() to Table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #11587:
URL: https://github.com/apache/iceberg/pull/11587#issuecomment-2545378279
closing this in favor of #11620
--
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] API, Core: Add formatVersion() to Table [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11587:
URL: https://github.com/apache/iceberg/pull/11587#discussion_r1848685646
##########
core/src/main/java/org/apache/iceberg/BaseMetadataTable.java:
##########
@@ -212,6 +212,11 @@ public String toString() {
return name();
}
+ @Override
+ public int formatVersion() {
+ return table().formatVersion();
Review Comment:
Do metadata tables have format versions? I would argue no?
--
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] API, Core: Add formatVersion() to Table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #11587:
URL: https://github.com/apache/iceberg/pull/11587#issuecomment-2491230705
I've created an alternative impl to this in https://github.com/apache/iceberg/pull/11620 where we can achieve the same thing without adjusting the `Table` API.
--
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