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 2022/04/06 06:00:13 UTC
[GitHub] [iceberg] ajantha-bhat opened a new pull request, #4509: Nessie: Fix NPE while accessing refreshed table in nessie catalog
ajantha-bhat opened a new pull request, #4509:
URL: https://github.com/apache/iceberg/pull/4509
what is null ?
`TableMetadata.metadataFileLocation` is null
Scenario:
```
create branch1;
create table1 on branch1;
commit on table1 on branch1;
create branch2 from branch1;
commit on table1 on branch2;
use branch1;
load table1 in branch1; -- this table as null `metadataFileLocation`
```
Reason:
`NessieTableOperation.refreshFromMetadataLocation()` calls `loadTableMetadata()` where `TableMetadata.buildFrom() ` creates an `TableMetadata` with null `metadataFileLocation` (refer [here](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/TableMetadata.java#L800))
and Nessie didn't set the `metadataFileLocation`
How basic scenarios are working?
Nessie itself holds `metadataFileLocation` and uses for all the Nessie operations. But when Iceberg table in nessie catalog is used for iceberg specific functionality like read `.snapshots` table, Iceberg needs `metadataFileLocation` which was not filled.
--
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
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #4509: Nessie: Fix NPE while accessing refreshed table in nessie catalog
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #4509:
URL: https://github.com/apache/iceberg/pull/4509#discussion_r843538016
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibility.java:
##########
@@ -173,6 +174,33 @@ public void testSchemaSnapshot() throws Exception {
Assertions.assertThat(metadataOn2).isNotEqualTo(metadataOnTest).isNotEqualTo(metadataOnTest2);
}
+ @Test
+ public void testMetadataLocationWithRefresh() throws Exception {
+ String branch1 = "test";
+ String branch2 = "branch-2";
+
+ // commit on tableIdentifier1 on branch1
+ NessieCatalog catalog = initCatalog(branch1);
+ String metadataCommit1 = addRow(catalog, tableIdentifier1, "initial-data",
Review Comment:
Renamed on newly added testcase, base testcases can be done in a follow up PR as it will confuse the reviewers.
--
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
[GitHub] [iceberg] ajantha-bhat commented on pull request #4509: Nessie: Fix NPE while accessing refreshed table in nessie catalog
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on PR #4509:
URL: https://github.com/apache/iceberg/pull/4509#issuecomment-1089854144
cc: @nastra , @snazy, @rdblue , @openinx
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #4509: Nessie: Fix NPE while accessing refreshed table in nessie catalog
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #4509:
URL: https://github.com/apache/iceberg/pull/4509#discussion_r843688073
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibility.java:
##########
@@ -173,6 +174,31 @@ public void testSchemaSnapshot() throws Exception {
Assertions.assertThat(metadataOn2).isNotEqualTo(metadataOnTest).isNotEqualTo(metadataOnTest2);
}
+ @Test
+ public void testMetadataLocation() throws Exception {
+ String branch1 = "test";
+ String branch2 = "branch-2";
+
+ // commit on tableIdentifier1 on branch1
+ NessieCatalog catalog = initCatalog(branch1);
+ String metadataLocationOfCommit1 = addRow(catalog, tableIdentifier1, "initial-data",
+ ImmutableMap.of("id0", 4L));
+
+ createBranch(branch2, catalog.currentHash(), branch1);
+ // commit on tableIdentifier1 on branch2
+ catalog = initCatalog(branch2);
+ String metadataLocationOfCommit2 = addRow(catalog, tableIdentifier1, "some-more-data",
+ ImmutableMap.of("id0", 42L));
+ Assertions.assertThat(metadataLocationOfCommit2).isNotEqualTo(metadataLocationOfCommit1);
Review Comment:
nit: maybe change to `.isNotNull().isNotEqualTo(metadataLocationOfCommit1)`. same for the other check further below
--
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
[GitHub] [iceberg] ajantha-bhat commented on pull request #4509: Nessie: Fix NPE while accessing refreshed table in nessie catalog
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on PR #4509:
URL: https://github.com/apache/iceberg/pull/4509#issuecomment-1091193076
@openinx : Thanks for merging.
--
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
[GitHub] [iceberg] openinx merged pull request #4509: Nessie: Fix NPE while accessing refreshed table in nessie catalog
Posted by GitBox <gi...@apache.org>.
openinx merged PR #4509:
URL: https://github.com/apache/iceberg/pull/4509
--
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
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #4509: Nessie: Fix NPE while accessing refreshed table in nessie catalog
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #4509:
URL: https://github.com/apache/iceberg/pull/4509#discussion_r843528494
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibility.java:
##########
@@ -173,6 +174,33 @@ public void testSchemaSnapshot() throws Exception {
Assertions.assertThat(metadataOn2).isNotEqualTo(metadataOnTest).isNotEqualTo(metadataOnTest2);
}
+ @Test
+ public void testMetadataLocationWithRefresh() throws Exception {
+ String branch1 = "test";
+ String branch2 = "branch-2";
+
+ // commit on tableIdentifier1 on branch1
+ NessieCatalog catalog = initCatalog(branch1);
+ String metadataCommit1 = addRow(catalog, tableIdentifier1, "initial-data",
+ ImmutableMap.of("id0", 4L));
+ catalog.refresh();
Review Comment:
it is just a double check that even refresh is not gonna fix it.
--
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
[GitHub] [iceberg] ajantha-bhat commented on pull request #4509: Nessie: Fix NPE while accessing refreshed table in nessie catalog
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on PR #4509:
URL: https://github.com/apache/iceberg/pull/4509#issuecomment-1090988129
@szehon-ho / @openinx : Can you please help in merging 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
[GitHub] [iceberg] nastra commented on a diff in pull request #4509: Nessie: Fix NPE while accessing refreshed table in nessie catalog
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #4509:
URL: https://github.com/apache/iceberg/pull/4509#discussion_r843612591
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibility.java:
##########
@@ -173,6 +174,33 @@ public void testSchemaSnapshot() throws Exception {
Assertions.assertThat(metadataOn2).isNotEqualTo(metadataOnTest).isNotEqualTo(metadataOnTest2);
}
+ @Test
+ public void testMetadataLocationWithRefresh() throws Exception {
+ String branch1 = "test";
+ String branch2 = "branch-2";
+
+ // commit on tableIdentifier1 on branch1
+ NessieCatalog catalog = initCatalog(branch1);
+ String metadataCommit1 = addRow(catalog, tableIdentifier1, "initial-data",
+ ImmutableMap.of("id0", 4L));
+ catalog.refresh();
Review Comment:
refresh() shouldn't be used outside of the internals of the catalog imo (I have a PR that cleans that up). Also it's not apparent that this is the reason why refresh() is being used here, so I woudl just remove it
--
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
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #4509: Nessie: Fix NPE while accessing refreshed table in nessie catalog
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #4509:
URL: https://github.com/apache/iceberg/pull/4509#discussion_r843539164
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibility.java:
##########
@@ -173,6 +174,33 @@ public void testSchemaSnapshot() throws Exception {
Assertions.assertThat(metadataOn2).isNotEqualTo(metadataOnTest).isNotEqualTo(metadataOnTest2);
}
+ @Test
+ public void testMetadataLocationWithRefresh() throws Exception {
+ String branch1 = "test";
+ String branch2 = "branch-2";
+
+ // commit on tableIdentifier1 on branch1
+ NessieCatalog catalog = initCatalog(branch1);
+ String metadataCommit1 = addRow(catalog, tableIdentifier1, "initial-data",
+ ImmutableMap.of("id0", 4L));
+ catalog.refresh();
+
+ createBranch(branch2, catalog.currentHash(), branch1);
+ // commit on tableIdentifier1 on branch2
+ NessieCatalog catalogBranch1 = initCatalog(branch2);
Review Comment:
I followed the pattern from base testcases in this file.
Changed this on newly added testcase, base testcases can be done in a follow up PR as it will confuse the reviewers.
--
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
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #4509: Nessie: Fix NPE while accessing refreshed table in nessie catalog
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #4509:
URL: https://github.com/apache/iceberg/pull/4509#discussion_r843620296
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibility.java:
##########
@@ -173,6 +174,33 @@ public void testSchemaSnapshot() throws Exception {
Assertions.assertThat(metadataOn2).isNotEqualTo(metadataOnTest).isNotEqualTo(metadataOnTest2);
}
+ @Test
+ public void testMetadataLocationWithRefresh() throws Exception {
+ String branch1 = "test";
+ String branch2 = "branch-2";
+
+ // commit on tableIdentifier1 on branch1
+ NessieCatalog catalog = initCatalog(branch1);
+ String metadataCommit1 = addRow(catalog, tableIdentifier1, "initial-data",
+ ImmutableMap.of("id0", 4L));
+ catalog.refresh();
Review Comment:
ok. Removed.
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #4509: Nessie: Fix NPE while accessing refreshed table in nessie catalog
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #4509:
URL: https://github.com/apache/iceberg/pull/4509#discussion_r843505767
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibility.java:
##########
@@ -173,6 +174,33 @@ public void testSchemaSnapshot() throws Exception {
Assertions.assertThat(metadataOn2).isNotEqualTo(metadataOnTest).isNotEqualTo(metadataOnTest2);
}
+ @Test
+ public void testMetadataLocationWithRefresh() throws Exception {
+ String branch1 = "test";
+ String branch2 = "branch-2";
+
+ // commit on tableIdentifier1 on branch1
+ NessieCatalog catalog = initCatalog(branch1);
+ String metadataCommit1 = addRow(catalog, tableIdentifier1, "initial-data",
+ ImmutableMap.of("id0", 4L));
+ catalog.refresh();
+
+ createBranch(branch2, catalog.currentHash(), branch1);
+ // commit on tableIdentifier1 on branch2
+ NessieCatalog catalogBranch1 = initCatalog(branch2);
Review Comment:
maybe just change to `catalog = initCatalog(branch2)`
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibility.java:
##########
@@ -173,6 +174,33 @@ public void testSchemaSnapshot() throws Exception {
Assertions.assertThat(metadataOn2).isNotEqualTo(metadataOnTest).isNotEqualTo(metadataOnTest2);
}
+ @Test
+ public void testMetadataLocationWithRefresh() throws Exception {
+ String branch1 = "test";
+ String branch2 = "branch-2";
+
+ // commit on tableIdentifier1 on branch1
+ NessieCatalog catalog = initCatalog(branch1);
+ String metadataCommit1 = addRow(catalog, tableIdentifier1, "initial-data",
Review Comment:
can we rename this (and others) to something like `metadataLocationOfCommit1`. Initially it wasn't clear that this refers to the metadata location
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibility.java:
##########
@@ -173,6 +174,33 @@ public void testSchemaSnapshot() throws Exception {
Assertions.assertThat(metadataOn2).isNotEqualTo(metadataOnTest).isNotEqualTo(metadataOnTest2);
}
+ @Test
+ public void testMetadataLocationWithRefresh() throws Exception {
+ String branch1 = "test";
+ String branch2 = "branch-2";
+
+ // commit on tableIdentifier1 on branch1
+ NessieCatalog catalog = initCatalog(branch1);
+ String metadataCommit1 = addRow(catalog, tableIdentifier1, "initial-data",
+ ImmutableMap.of("id0", 4L));
+ catalog.refresh();
Review Comment:
those `catalog.refresh()` calls can be removed
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #4509: Nessie: Fix NPE while accessing refreshed table in nessie catalog
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #4509:
URL: https://github.com/apache/iceberg/pull/4509#discussion_r843507101
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibility.java:
##########
@@ -173,6 +174,33 @@ public void testSchemaSnapshot() throws Exception {
Assertions.assertThat(metadataOn2).isNotEqualTo(metadataOnTest).isNotEqualTo(metadataOnTest2);
}
+ @Test
+ public void testMetadataLocationWithRefresh() throws Exception {
+ String branch1 = "test";
+ String branch2 = "branch-2";
+
+ // commit on tableIdentifier1 on branch1
+ NessieCatalog catalog = initCatalog(branch1);
+ String metadataCommit1 = addRow(catalog, tableIdentifier1, "initial-data",
+ ImmutableMap.of("id0", 4L));
+ catalog.refresh();
+
+ createBranch(branch2, catalog.currentHash(), branch1);
+ // commit on tableIdentifier1 on branch2
+ NessieCatalog catalogBranch1 = initCatalog(branch2);
+ String metadataCommit2 = addRow(catalogBranch1, tableIdentifier1, "some-more-data",
+ ImmutableMap.of("id0", 42L));
+ Assertions.assertThat(metadataCommit2).isNotEqualTo(metadataCommit1);
+
+ catalog = initCatalog(branch1);
+ catalog.refresh();
+ // load tableIdentifier1 on branch1
+ BaseTable table = (BaseTable)catalog.loadTable(tableIdentifier1);
Review Comment:
nit: formatting
--
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