Skip to content

Nessie: Fix NPE while accessing refreshed table in nessie catalog #4509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
openinx merged 1 commit into from
Apr 7, 2022
Merged

Nessie: Fix NPE while accessing refreshed table in nessie catalog #4509

openinx merged 1 commit into from
Apr 7, 2022

Conversation

@ajantha-bhat
Copy link
Member

@ajantha-bhat ajantha-bhat commented Apr 6, 2022 •
edited
Loading

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 has null `metadataFileLocation`

Reason:
NessieTableOperation.refreshFromMetadataLocation() calls loadTableMetadata() where TableMetadata.buildFrom() creates an TableMetadata with null metadataFileLocation (refer here)
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.

@github-actions github-actions bot added the NESSIE label Apr 6, 2022
@ajantha-bhat
Copy link
Member Author

ajantha-bhat commented Apr 6, 2022

cc: @nastra , @snazy, @rdblue , @openinx

nastra
nastra reviewed Apr 6, 2022
View reviewed changes
nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibility.java Outdated

createBranch(branch2, catalog.currentHash(), branch1);
// commit on tableIdentifier1 on branch2
NessieCatalog catalogBranch1 = initCatalog(branch2);
Copy link
Contributor

@nastra nastra Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just change to catalog = initCatalog(branch2)

Copy link
Member Author

@ajantha-bhat ajantha-bhat Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibility.java Outdated
NessieCatalog catalog = initCatalog(branch1);
String metadataCommit1 = addRow(catalog, tableIdentifier1, "initial-data",
ImmutableMap.of("id0", 4L));
catalog.refresh();
Copy link
Contributor

@nastra nastra Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those catalog.refresh() calls can be removed

Copy link
Member Author

@ajantha-bhat ajantha-bhat Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is just a double check that even refresh is not gonna fix it.

Copy link
Contributor

@nastra nastra Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@ajantha-bhat ajantha-bhat Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. Removed.

nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibility.java Outdated

// commit on tableIdentifier1 on branch1
NessieCatalog catalog = initCatalog(branch1);
String metadataCommit1 = addRow(catalog, tableIdentifier1, "initial-data",
Copy link
Contributor

@nastra nastra Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we rename this (and others) to something like metadataLocationOfCommit1. Initially it wasn't clear that this refers to the metadata location

Copy link
Member Author

@ajantha-bhat ajantha-bhat Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed on newly added testcase, base testcases can be done in a follow up PR as it will confuse the reviewers.

nastra
nastra reviewed Apr 6, 2022
View reviewed changes
nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibility.java Outdated
catalog = initCatalog(branch1);
catalog.refresh();
// load tableIdentifier1 on branch1
BaseTable table = (BaseTable)catalog.loadTable(tableIdentifier1);
Copy link
Contributor

@nastra nastra Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: formatting

@ajantha-bhat ajantha-bhat force-pushed the npe branch 2 times, most recently from 75da1a7 to 819e751 Compare April 6, 2022 08:07
nastra
nastra approved these changes Apr 6, 2022
View reviewed changes
nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibility.java Outdated
catalog = initCatalog(branch2);
String metadataLocationOfCommit2 = addRow(catalog, tableIdentifier1, "some-more-data",
ImmutableMap.of("id0", 42L));
Assertions.assertThat(metadataLocationOfCommit2).isNotEqualTo(metadataLocationOfCommit1);
Copy link
Contributor

@nastra nastra Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe change to .isNotNull().isNotEqualTo(metadataLocationOfCommit1). same for the other check further below

@ajantha-bhat
Copy link
Member Author

ajantha-bhat commented Apr 7, 2022

@szehon-ho / @openinx : Can you please help in merging this ?

openinx
openinx approved these changes Apr 7, 2022
View reviewed changes
Copy link
Member

@openinx openinx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me .

@openinx openinx merged commit 4b382e2 into apache:master Apr 7, 2022
@ajantha-bhat
Copy link
Member Author

ajantha-bhat commented Apr 7, 2022

@openinx : Thanks for merging.

@nastra nastra added this to the Iceberg 0.13.2 Release milestone May 23, 2022
nastra pushed a commit to nastra/iceberg that referenced this pull request May 23, 2022
@ajantha-bhat @nastra
Nessie: Fix NPE while accessing refreshed table in nessie catalog (ap…
72f1d85 
…ache#4509)
@nastra nastra mentioned this pull request May 23, 2022
Merged
XN137 added a commit to XN137/nessie-demos that referenced this pull request May 23, 2022
@XN137
Upgrade demos to nessie 0.19.0
4368546 
- upgrade to iceberg 0.13.1
- upgrade to flink 1.13.6
- update notebooks to new sql grammar
  (see projectnessie/nessie#2273)
- adjust notebooks to new nessie cli
- adjust notebooks to new nessie python api
- adjust notebooks to no longer claim branches have the same
  hash after merging
- avoid silent exception handling
- apply workaround for delta demo
  (for projectnessie/nessie#3552)
- apply workaround for hive demo
  (for apache/iceberg#4509)
@XN137 XN137 mentioned this pull request May 23, 2022
Merged
XN137 added a commit to XN137/nessie-demos that referenced this pull request May 23, 2022
@XN137
Upgrade demos to nessie 0.19.0
cc70d1f 
- upgrade to iceberg 0.13.1
- upgrade to flink 1.13.6
- update notebooks to new sql grammar
  (see projectnessie/nessie#2273)
- adjust notebooks to new nessie cli
- adjust notebooks to new nessie python api
- adjust notebooks to no longer claim branches have the same
  hash after merging
- avoid silent exception handling
- apply workaround for delta demo
  (for projectnessie/nessie#3552)
- apply workaround for hive demo
  (for apache/iceberg#4509)
rdblue pushed a commit that referenced this pull request May 23, 2022
@nastra @ajantha-bhat
Nessie: Fix NPE while accessing refreshed table in nessie catalog (#4509
bcc036e 
) (#4840)

Co-authored-by: Ajantha Bhat <[email protected]>
XN137 added a commit to projectnessie/nessie-demos that referenced this pull request May 24, 2022
@XN137
Upgrade demos to nessie 0.19.0 (#334)
8ab8587 
- upgrade to iceberg 0.13.1
- upgrade to flink 1.13.6
- update notebooks to new sql grammar
  (see projectnessie/nessie#2273)
- adjust notebooks to new nessie cli
- adjust notebooks to new nessie python api
- adjust notebooks to no longer claim branches have the same
  hash after merging
- avoid silent exception handling
- apply workaround for delta demo
  (for projectnessie/nessie#3552)
- apply workaround for hive demo
  (for apache/iceberg#4509)

closes #260
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nastra nastra nastra approved these changes

@openinx openinx openinx approved these changes

Assignees

No one assigned

Labels

NESSIE

Projects

None yet

Milestone

Iceberg 0.13.2 Release

Development

Successfully merging this pull request may close these issues.

3 participants

@ajantha-bhat @nastra @openinx