Skip to content

HIVE-28765: Iceberg: Incorrect partition statistics on time travel + partition evolution - #5748

Merged
okumin merged 5 commits into
apache:masterfrom
okumin:HIVE-28765-partition-snapshot
Apr 23, 2025
Merged

HIVE-28765: Iceberg: Incorrect partition statistics on time travel + partition evolution#5748
okumin merged 5 commits into
apache:masterfrom
okumin:HIVE-28765-partition-snapshot

Conversation

@okumin

@okumin okumin commented Apr 6, 2025

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

https://issues.apache.org/jira/browse/HIVE-28765

We use not partition stats but table stats when we access a complicated table with tagging or branching.

Why are the changes needed?

We would like to improve the query plan for a time-travel read with tagging and branching.

Does this PR introduce any user-facing change?

No

Is the change a dependency upgrade?

No

How was this patch tested?

Updated an existing query file.

@okumin okumin changed the title [WIP] HIVE-28765: Iceberg: Incorrect partition statistics on time travel + partition evolution HIVE-28765: Iceberg: Incorrect partition statistics on time travel + partition evolution Apr 12, 2025
@okumin
okumin marked this pull request as ready for review April 12, 2025 07:34
}
Table table = IcebergTableUtil.getTable(conf, hmsTable.getTTable());
Snapshot snapshot = IcebergTableUtil.getTableSnapshot(table, hmsTable);
boolean isTimeTravel = snapshot != null && table.currentSnapshot() != null &&

@deniskuzZ deniskuzZ Apr 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@okumin, could we use equals here?

  public boolean isPartitioned(org.apache.hadoop.hive.ql.metadata.Table hmsTable) {
    if (!hmsTable.getTTable().isSetId()) {
      return false;
    }
    Table table = IcebergTableUtil.getTable(conf, hmsTable.getTTable());
    Snapshot snapshot = IcebergTableUtil.getTableSnapshot(table, hmsTable);
    
    boolean isTimeTravelOrBranch = snapshot != null && snapshot.equals(table.currentSnapshot());
    if (isTimeTravelOrBranch && hasUndergonePartitionEvolution(table)) {
      return false;
    }
    return table.spec().isPartitioned();
  }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree that it is simpler and tolerant with unexpected snapshots, such as having the same snapshot id but different timestamps.
https://github.com/apache/iceberg/blob/c338323b2e9cd8a862fdf328ceacc1b59ea6275a/core/src/main/java/org/apache/iceberg/BaseSnapshot.java#L320-L332

I updated it. I kept the variable name, isTimeTravel, assuming reading a different snapshot using tag/branch sounds like a time-travel query.

@deniskuzZ deniskuzZ Apr 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

forked branch could be ahead of main, so I wouldn't call it time-travel

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I grabbed your point. I applied a slight modification to the variable name.
1ac16b2

@deniskuzZ deniskuzZ left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, pending tests

@sonarqubecloud

Copy link
Copy Markdown

@okumin
okumin merged commit 28abf17 into apache:master Apr 23, 2025
@okumin
okumin deleted the HIVE-28765-partition-snapshot branch April 23, 2025 11:23
@okumin

okumin commented Apr 23, 2025

Copy link
Copy Markdown
Contributor Author

@deniskuzZ Thanks for reviewing this pull request!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants