You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/03/31 06:49:19 UTC
[GitHub] [iceberg] aokolnychyi opened a new pull request #2398: API: Add metadataFileLocation to Table
aokolnychyi opened a new pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398
This PR adds `metadataFileLocation` to `Table` so that we can leverage it in `SerializedTable` later on.
--
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.
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
[GitHub] [iceberg] aokolnychyi commented on pull request #2398: API: Add metadataFileLocation to Table
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#issuecomment-810822067
cc @pvary @rdblue @RussellSpitzer @yyanyy @jackye1995
--
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.
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
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2398: API: Add metadataFileLocation to Table
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r605244537
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetadataTable.java
##########
@@ -37,8 +37,30 @@
abstract class BaseMetadataTable implements Table, Serializable {
private final PartitionSpec spec = PartitionSpec.unpartitioned();
private final SortOrder sortOrder = SortOrder.unsorted();
+ private final TableOperations ops;
+ private final Table table;
+ private final String name;
- abstract Table table();
+ protected BaseMetadataTable(TableOperations ops, Table table, String name) {
Review comment:
I can also understand if we don't ops protected and not public
--
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.
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
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2398: API: Add metadataFileLocation to Table
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r604637843
##########
File path: core/src/main/java/org/apache/iceberg/AllDataFilesTable.java
##########
@@ -40,51 +40,31 @@
* This table may return duplicate rows.
*/
public class AllDataFilesTable extends BaseMetadataTable {
- private final TableOperations ops;
Review comment:
Instead of modifying `metadataLocation` in each metadata table, I added `ops` and `table` to the parent class and implemented needed methods there. I can put the refactoring into a separate PR if that simplifies the review.
--
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.
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
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2398: API: Add metadataFileLocation to Table
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r604637920
##########
File path: api/src/main/java/org/apache/iceberg/Table.java
##########
@@ -102,6 +102,15 @@ default String name() {
*/
String location();
+ /**
+ * Returns the location of the current metadata file.
+ *
+ * @return this table's metadata file location
+ */
+ default String metadataFileLocation() {
+ return null;
Review comment:
For backward compatibility.
--
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.
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
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2398: API: Add metadataFileLocation to Table
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r604900010
##########
File path: api/src/main/java/org/apache/iceberg/Table.java
##########
@@ -102,6 +102,15 @@ default String name() {
*/
String location();
+ /**
+ * Returns the location of the current metadata file.
+ *
+ * @return this table's metadata file location
+ */
+ default String metadataFileLocation() {
+ return null;
Review comment:
Shouldn't it be not implemented exception? Or is it ok for this to return null sometimes?
--
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.
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
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2398: Core: Implement HasTableOperations in metadata and txn tables
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r605308464
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetadataTable.java
##########
@@ -34,11 +34,34 @@
* using a {@link StaticTableOperations}. This way no Catalog related calls are needed when reading the table data after
* deserialization.
*/
-abstract class BaseMetadataTable implements Table, Serializable {
+abstract class BaseMetadataTable implements Table, HasTableOperations, Serializable {
private final PartitionSpec spec = PartitionSpec.unpartitioned();
private final SortOrder sortOrder = SortOrder.unsorted();
+ private final TableOperations ops;
+ private final Table table;
+ private final String name;
- abstract Table table();
+ protected BaseMetadataTable(TableOperations ops, Table table, String name) {
+ this.ops = ops;
+ this.table = table;
+ this.name = name;
+ }
+
+ abstract MetadataTableType metadataTableType();
+
+ protected Table table() {
+ return table;
+ }
+
+ @Override
+ public TableOperations operations() {
Review comment:
Here we are exposing whatever ops we get in the metadata table. You are right it is currently the same table ops as in the base table. I think that was done intentionally to keep metadata tables in sync with the main table. However, we cannot guarantee this won't change in the future. In `BaseMetadataTable`, we don't really know what ops we got.
--
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.
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
[GitHub] [iceberg] aokolnychyi edited a comment on pull request #2398: Core: Implement HasTableOperations in metadata and txn tables
Posted by GitBox <gi...@apache.org>.
aokolnychyi edited a comment on pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#issuecomment-811524385
@pvary @rdblue @RussellSpitzer @yyanyy @jackye1995, I've changed the approach. No more public changes.
--
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.
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
[GitHub] [iceberg] pvary commented on pull request #2398: Core: Implement HasTableOperations in metadata and txn tables
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#issuecomment-814739180
Late +1. I was thinking about the same things when did the serialization stuff, but skipped this because of the time pressure then (and later forgot about it). Thanks for taking care of this @aokolnychyi.
--
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.
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
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2398: Core: Implement HasTableOperations in metadata and txn tables
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r605269543
##########
File path: api/src/main/java/org/apache/iceberg/Table.java
##########
@@ -102,6 +102,15 @@ default String name() {
*/
String location();
+ /**
+ * Returns the location of the current metadata file.
+ *
+ * @return this table's metadata file location
+ */
+ default String metadataFileLocation() {
+ return null;
Review comment:
Reverted 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.
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
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2398: Core: Implement HasTableOperations in metadata and txn tables
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r605270100
##########
File path: core/src/main/java/org/apache/iceberg/AllDataFilesTable.java
##########
@@ -40,51 +40,31 @@
* This table may return duplicate rows.
*/
public class AllDataFilesTable extends BaseMetadataTable {
- private final TableOperations ops;
Review comment:
No longer applies. We need this changes.
--
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.
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
[GitHub] [iceberg] aokolnychyi commented on pull request #2398: Core: Implement HasTableOperations in metadata and txn tables
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#issuecomment-811575706
Thanks for reviewing, @RussellSpitzer @yyanyy @rdblue!
--
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.
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
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2398: API: Add metadataFileLocation to Table
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r604637843
##########
File path: core/src/main/java/org/apache/iceberg/AllDataFilesTable.java
##########
@@ -40,51 +40,31 @@
* This table may return duplicate rows.
*/
public class AllDataFilesTable extends BaseMetadataTable {
- private final TableOperations ops;
Review comment:
Instead of modifying `metadataLocation` in each metadata table, I added `ops` and `table` to the parent class and implemented needed methods there. I can put the refactoring into a separate PR if that simplifies the review.
However, I'd want to know if everyone agrees with the overall approach first.
--
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.
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
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2398: API: Add metadataFileLocation to Table
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r605243222
##########
File path: api/src/main/java/org/apache/iceberg/Table.java
##########
@@ -102,6 +102,15 @@ default String name() {
*/
String location();
+ /**
+ * Returns the location of the current metadata file.
+ *
+ * @return this table's metadata file location
+ */
+ default String metadataFileLocation() {
+ return null;
Review comment:
Yeah my basic thought here was that you shouldn't be calling this if you are ok with it being null, so we should throw an error
--
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.
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
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2398: Core: Implement HasTableOperations in metadata and txn tables
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r605269864
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetadataTable.java
##########
@@ -37,8 +37,30 @@
abstract class BaseMetadataTable implements Table, Serializable {
private final PartitionSpec spec = PartitionSpec.unpartitioned();
private final SortOrder sortOrder = SortOrder.unsorted();
+ private final TableOperations ops;
+ private final Table table;
+ private final String name;
- abstract Table table();
+ protected BaseMetadataTable(TableOperations ops, Table table, String name) {
Review comment:
Yeah, I actually implemented `HasTableOperations` in both metadata and txn tables. I think that's a better alternative.
--
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.
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
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2398: Core: Implement HasTableOperations in metadata and txn tables
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r605308464
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetadataTable.java
##########
@@ -34,11 +34,34 @@
* using a {@link StaticTableOperations}. This way no Catalog related calls are needed when reading the table data after
* deserialization.
*/
-abstract class BaseMetadataTable implements Table, Serializable {
+abstract class BaseMetadataTable implements Table, HasTableOperations, Serializable {
private final PartitionSpec spec = PartitionSpec.unpartitioned();
private final SortOrder sortOrder = SortOrder.unsorted();
+ private final TableOperations ops;
+ private final Table table;
+ private final String name;
- abstract Table table();
+ protected BaseMetadataTable(TableOperations ops, Table table, String name) {
+ this.ops = ops;
+ this.table = table;
+ this.name = name;
+ }
+
+ abstract MetadataTableType metadataTableType();
+
+ protected Table table() {
+ return table;
+ }
+
+ @Override
+ public TableOperations operations() {
Review comment:
Here we are exposing whatever ops we get in the metadata table. You are right it is currently the same table ops as in the base table. I think that was done intentionally to keep metadata tables in sync. However, we cannot guarantee this won't change in the future. In `BaseMetadataTable`, we don't really know what ops we got.
--
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.
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
[GitHub] [iceberg] yyanyy commented on a change in pull request #2398: Core: Implement HasTableOperations in metadata and txn tables
Posted by GitBox <gi...@apache.org>.
yyanyy commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r605288411
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetadataTable.java
##########
@@ -34,11 +34,34 @@
* using a {@link StaticTableOperations}. This way no Catalog related calls are needed when reading the table data after
* deserialization.
*/
-abstract class BaseMetadataTable implements Table, Serializable {
+abstract class BaseMetadataTable implements Table, HasTableOperations, Serializable {
private final PartitionSpec spec = PartitionSpec.unpartitioned();
private final SortOrder sortOrder = SortOrder.unsorted();
+ private final TableOperations ops;
+ private final Table table;
+ private final String name;
- abstract Table table();
+ protected BaseMetadataTable(TableOperations ops, Table table, String name) {
+ this.ops = ops;
+ this.table = table;
+ this.name = name;
+ }
+
+ abstract MetadataTableType metadataTableType();
+
+ protected Table table() {
+ return table;
+ }
+
+ @Override
+ public TableOperations operations() {
Review comment:
I guess instead of exposing the metadata table's operation, here we are actually exposing the original base table's operations, wondering if it's worth adding a comment to explain
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetadataTable.java
##########
@@ -195,12 +218,9 @@ public String toString() {
return name();
}
- abstract String metadataLocation();
Review comment:
Is the plan to use `operations()` on metadata tables to derive metadata location? wondering if we want to keep this method for backward compatibility/avoid breaking consumer
--
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.
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
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2398: API: Add metadataFileLocation to Table
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r605244238
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetadataTable.java
##########
@@ -37,8 +37,30 @@
abstract class BaseMetadataTable implements Table, Serializable {
private final PartitionSpec spec = PartitionSpec.unpartitioned();
private final SortOrder sortOrder = SortOrder.unsorted();
+ private final TableOperations ops;
+ private final Table table;
+ private final String name;
- abstract Table table();
+ protected BaseMetadataTable(TableOperations ops, Table table, String name) {
Review comment:
should this also implement "hasTableOperations" then?
--
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.
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
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2398: Core: Implement HasTableOperations in metadata and txn tables
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r605270367
##########
File path: core/src/main/java/org/apache/iceberg/AllDataFilesTable.java
##########
@@ -40,51 +40,31 @@
* This table may return duplicate rows.
*/
public class AllDataFilesTable extends BaseMetadataTable {
- private final TableOperations ops;
Review comment:
Every metadata table has these. I moved to the parent class.
--
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.
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
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2398: API: Add metadataFileLocation to Table
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r604638355
##########
File path: core/src/main/java/org/apache/iceberg/ManifestsTable.java
##########
@@ -93,13 +74,13 @@ MetadataTableType metadataTableType() {
protected DataTask task(TableScan scan) {
String location = scan.snapshot().manifestListLocation();
return StaticDataTask.of(
- ops.io().newInputFile(location != null ? location : ops.current().metadataFileLocation()),
+ ops().io().newInputFile(location != null ? location : ops().current().metadataFileLocation()),
scan.snapshot().allManifests(),
manifest -> ManifestsTable.manifestFileToRow(spec, manifest));
}
private class ManifestsTableScan extends StaticTableScan {
- ManifestsTableScan() {
+ ManifestsTableScan(TableOperations ops, Table table) {
Review comment:
Won't compile otherwise.
--
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.
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
[GitHub] [iceberg] aokolnychyi merged pull request #2398: Core: Implement HasTableOperations in metadata and txn tables
Posted by GitBox <gi...@apache.org>.
aokolnychyi merged pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398
--
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.
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
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2398: API: Add metadataFileLocation to Table
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r604637843
##########
File path: core/src/main/java/org/apache/iceberg/AllDataFilesTable.java
##########
@@ -40,51 +40,31 @@
* This table may return duplicate rows.
*/
public class AllDataFilesTable extends BaseMetadataTable {
- private final TableOperations ops;
Review comment:
Instead of modifying `metadataLocation` in each metadata table, I added `ops` and `table` to the parent class and implemented needed methods there. I can put the refactoring into a separate PR if that simplifies the review.
However, I'd want to know if everyone agrees with that.
--
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.
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
[GitHub] [iceberg] rdblue commented on a change in pull request #2398: Core: Implement HasTableOperations in metadata and txn tables
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r605306807
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetadataTable.java
##########
@@ -195,12 +218,9 @@ public String toString() {
return name();
}
- abstract String metadataLocation();
Review comment:
This was package-private so it should be okay to remove it. I think using `HasTableOperations` is a better way to access the metadata location.
--
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.
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
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2398: Core: Implement HasTableOperations in metadata and txn tables
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r605309634
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetadataTable.java
##########
@@ -195,12 +218,9 @@ public String toString() {
return name();
}
- abstract String metadataLocation();
Review comment:
I don't think `BaseMetadataTable` is public and this method was added for serialization. I am going to rework table serialization in the next PR and we won't depend on this method.
That said, I don't mind keeping this too if there is a good use case. I just thought it is a pretty internal 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.
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
[GitHub] [iceberg] aokolnychyi commented on pull request #2398: API: Add metadataFileLocation to Table
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#issuecomment-811509624
I am going to switch to using `HasTableOperations` to not expose anything new. Let me try that locally and see.
--
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.
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
[GitHub] [iceberg] aokolnychyi commented on pull request #2398: Core: Implement HasTableOperations in metadata and txn tables
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#issuecomment-811524385
@pvary @rdblue @RussellSpitzer @yyanyy @jackye1995, I've changed the approach.
--
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.
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
[GitHub] [iceberg] yyanyy commented on a change in pull request #2398: Core: Implement HasTableOperations in metadata and txn tables
Posted by GitBox <gi...@apache.org>.
yyanyy commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r605316081
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetadataTable.java
##########
@@ -195,12 +218,9 @@ public String toString() {
return name();
}
- abstract String metadataLocation();
Review comment:
makes sense, didn't notice that it was package-private so probably don't need to add back. Thanks for the explanation!
--
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.
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
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2398: API: Add metadataFileLocation to Table
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2398:
URL: https://github.com/apache/iceberg/pull/2398#discussion_r605116102
##########
File path: api/src/main/java/org/apache/iceberg/Table.java
##########
@@ -102,6 +102,15 @@ default String name() {
*/
String location();
+ /**
+ * Returns the location of the current metadata file.
+ *
+ * @return this table's metadata file location
+ */
+ default String metadataFileLocation() {
+ return null;
Review comment:
I was thinking about this one too. Probably, an exception would be more descriptive? I am going to use it in the serializable table, where it will be optional. So null there is no problem but I am not sure about general cases.
--
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.
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