Skip to content
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.

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

[GOBBLIN-2169] Fix NPE while overwrite if table has no previous snapshot #4071

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

Blazer-007
Copy link
Contributor

@Blazer-007 Blazer-007 commented Oct 30, 2024

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

Description

  • [✔️ ] Here are some details about my PR, including screenshots (if applicable):
    While overwrite we log the snapshot id of last snapshot present before commit but in case of table with no snapshot (possible freshly created table) currentSnapshot will be null hence will give NPE if used to fetch SnapshotId
    This PR fixes that with proper logging.

Tests

  • [✔️ ] My PR adds the following unit tests OR does not need testing for this extremely good reason:
    NA

Commits

  • [✔️ ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

@Blazer-007 Blazer-007 changed the title [GOBBLIN-] Fix NPE while overwrite if table has no previous snapshot [GOBBLIN-2169] Fix NPE while overwrite if table has no previous snapshot Oct 30, 2024
@Will-Lo Will-Lo merged commit edaf474 into apache:master Oct 31, 2024
6 checks passed
Comment on lines 239 to 240
public List<DataFile> getPartitionSpecificDataFiles(Predicate<StructLike> icebergPartitionFilterPredicate)
throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't the last line here also throw an NPE for the same reason?

  public List<DataFile> getPartitionSpecificDataFiles(Predicate<StructLike> icebergPartitionFilterPredicate)
      throws IOException {
    TableMetadata tableMetadata = accessTableMetadata();
    Snapshot currentSnapshot = tableMetadata.currentSnapshot();
    long currentSnapshotId = currentSnapshot.snapshotId();

looking quickly at this class, there are several places that are not resilient to the unexpected circumstance of a table having existing and metadata, but not actually any snapshots yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Blazer-007 Blazer-007 deleted the virai_partition_npe_fix branch December 9, 2024 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants