Skip to content

Make Iceberg materialized view refresh atomic #14145

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

Merged
findepi merged 1 commit into from
Sep 16, 2022
Merged

Make Iceberg materialized view refresh atomic #14145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions docs/src/main/sphinx/connector/iceberg.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -985,11 +985,5 @@ view is queried, the snapshot-ids are used to check if the data in the storage
table is up to date. If the data is outdated, the materialized view behaves
like a normal view, and the data is queried directly from the base tables.

.. warning::

There is a small time window between the commit of the delete and insert,
when the materialized view is empty. If the commit operation for the insert
fails, the materialized view remains empty.

Dropping a materialized view with :doc:`/sql/drop-materialized-view` removes
the definition and the storage table.
8 changes: 5 additions & 3 deletions plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2147,12 +2147,14 @@ public Optional<ConnectorOutputMetadata> finishRefreshMaterializedView(
Collection<ComputedStatistics> computedStatistics,
List<ConnectorTableHandle> sourceTableHandles)
{
// delete before insert .. simulating overwrite
executeDelete(session, tableHandle);

IcebergWritableTableHandle table = (IcebergWritableTableHandle) insertHandle;

Table icebergTable = transaction.table();
// delete before insert .. simulating overwrite
transaction.newDelete()
.deleteFromRowFilter(Expressions.alwaysTrue())
Copy link
Copy Markdown
Member Author

@alexjo2144 alexjo2144 Sep 15, 2022

Choose a reason for hiding this comment

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

@raunaqmorarka I wasn't totally sure about this alwaysTrue part. Do we always rewrite the entire content of a MV?

Copy link
Copy Markdown
Member

@findepi findepi Sep 15, 2022

Choose a reason for hiding this comment

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

I think incremental refreshes are supported only if io.trino.spi.connector.ConnectorMetadata#delegateMaterializedViewRefreshToConnector returns true (it returns false for Iceberg), and connector knows how to refresh incrementally.

BTW @alexjo2144 will this unregister all old data files, or will it scan any data file contents?

Also, does this preserve table version history correctly?

Copy link
Copy Markdown
Member Author

@alexjo2144 alexjo2144 Sep 15, 2022

Choose a reason for hiding this comment

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

BTW @alexjo2144 will this unregister all old data files, or will it scan any data file contents?

This will unregister the data files from the manifest and they will not be scanned. It is the same code as is run by executeDelete just attached to the transaction instead of being run separately.

Also, does this preserve table version history correctly?

If we need history to be retained we should upgrade to 0.14.1 first, there's a bug fix in that we'd need. However, I was assuming history for an MV is not useful

Copy link
Copy Markdown
Member

@findepi findepi Sep 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If we need history to be retained we should upgrade to 0.14.1 first, there's a bug fix in that we'd need

yeah, just realized it's coming now: #14074 (comment)

However, I was assuming history for an MV is not useful

perhaps; i am fine merging as is, without 0.14.1

.commit();

List<CommitTaskData> commitTasks = fragments.stream()
.map(slice -> commitTaskCodec.fromJson(slice.getBytes()))
.collect(toImmutableList());
Expand Down
4 changes: 2 additions & 2 deletions ...o-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMetastoreAccessOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,8 @@ public void testRefreshMaterializedView()

assertMetastoreInvocations("REFRESH MATERIALIZED VIEW test_refresh_mview_view",
ImmutableMultiset.builder()
.addCopies(GET_TABLE, 9)
.addCopies(REPLACE_TABLE, 2)
.addCopies(GET_TABLE, 6)
.addCopies(REPLACE_TABLE, 1)
.build());
}

Expand Down