Skip to content

Iceberg connector support for Materialized Views#4832

Merged
electrum merged 5 commits intotrinodb:masterfrom
anjalinorwood:matview_iceberg
Nov 9, 2020
Merged

Iceberg connector support for Materialized Views#4832
electrum merged 5 commits intotrinodb:masterfrom
anjalinorwood:matview_iceberg

Conversation

@anjalinorwood
Copy link
Member

This PR is for Iceberg connector changes to support materialized views. Note that only the top commit (
25678de) needs to be reviewed as a part of this PR. Other commits are being reviewed here: #3283

@cla-bot cla-bot bot added the cla-signed label Aug 14, 2020
@anjalinorwood anjalinorwood requested a review from martint August 14, 2020 06:13
@anjalinorwood anjalinorwood force-pushed the matview_iceberg branch 3 times, most recently from a196b29 to 4e43768 Compare August 20, 2020 16:01
@anjalinorwood anjalinorwood force-pushed the matview_iceberg branch 4 times, most recently from b97176c to 3d35309 Compare August 31, 2020 20:32
@martint martint requested a review from electrum August 31, 2020 21:15
@electrum
Copy link
Member

electrum commented Sep 1, 2020

@anjalinorwood can you rebase now that the materialized view code is merged?

@anjalinorwood
Copy link
Member Author

anjalinorwood commented Sep 1, 2020 via email

@anjalinorwood
Copy link
Member Author

@martint and @electrum Did you get a chance to review this PR? Thanks.

Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

Some initial comments, mostly superficial. Still reviewing

@anjalinorwood anjalinorwood force-pushed the matview_iceberg branch 2 times, most recently from 85c58a6 to e7298a3 Compare October 1, 2020 05:45
@anjalinorwood anjalinorwood force-pushed the matview_iceberg branch 2 times, most recently from 03b2126 to ccf250b Compare October 7, 2020 20:39
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

A bunch of minor comments. Overall looks good

@anjalinorwood
Copy link
Member Author

@martint Did you want to give this a look? I think @electrum is winding up his review. Thank you both.

Copy link
Member

@martint martint left a comment

Choose a reason for hiding this comment

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

I reviewed all commits except for "Iceberg connector support for Materialized Views". Just a couple of minor comments.

@electrum, can you finish reviewing that other commit?

... clauses together in 'CREATE MATERIALIZED VIEW' statement.

The optional 'IF NOT EXISTS' clause causes the error to be suppressed
if a materialized view already exists. This clause was silently ignored
if 'OR REPLACE' was specified.
This can be confusing to users, so this commit explicitly throws an error
when the two clauses are specified together.
Even though materialized views are accessible through a connector and can be said to
'belong' to a connector, they need to be treated separately from tables. Connector
APIs for materialized views that modeled them as TableHandles are being changed to
pass in materialized view name rather than handles.
Iceberg connector allows creation, refresh and drop of materialized views.
It provides methods to determine if a given materialized view is current
with respect to underlying base table(s).
Freshness of a materialized view is determined by comparing the snapshot ID
of base table(s) at the time the materialized view was refreshed with
snapshot ID of base table(s) at the time of the check.
@electrum electrum merged commit a8f94f0 into trinodb:master Nov 9, 2020
@electrum
Copy link
Member

electrum commented Nov 9, 2020

Thanks!

@electrum electrum mentioned this pull request Nov 10, 2020
10 tasks
@electrum electrum added this to the 346 milestone Nov 10, 2020
@findepi
Copy link
Member

findepi commented Nov 10, 2020

#5892

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

Development

Successfully merging this pull request may close these issues.

4 participants