-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Allow Iceberg materialized views to federate #15108
Conversation
d70c278
to
446d187
Compare
There was a problem hiding this 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. I leave the approval to other reviewers who're familiar with materialized views.
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Outdated
Show resolved
Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java
Outdated
Show resolved
Hide resolved
446d187
to
02c5a64
Compare
thank you @ebyhr for your review. AC. PTAL |
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Show resolved
Hide resolved
e9f0619
to
1ba2559
Compare
@raunaqmorarka added tests reproducing the problem and addressed We have effectively three different states an MV can be in
To support this, i changed |
...ino-tests/src/test/java/io/trino/connector/informationschema/BenchmarkInformationSchema.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/metadata/TestInformationSchemaMetadata.java
Outdated
Show resolved
Hide resolved
testing/trino-tests/src/test/java/io/trino/execution/TestEventListenerBasic.java
Outdated
Show resolved
Hide resolved
testing/trino-tests/src/test/java/io/trino/execution/TestEventListenerBasic.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/connector/system/MaterializedViewSystemTable.java
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Show resolved
Hide resolved
create table iceberg.default.itable1 (x integer);
insert into iceberg.default.itable1 values (1),(2),(3);
create materialized view iceberg.default.mvtest1
as select nationkey, name
from iceberg.default.itable1 itable1
inner join tpch.sf1.nation tnation on itable1.x = tnation.nationkey;
refresh maerialized view iceberg.default.mvtest1;
select * from iceberg.default.mvtest1 ;
insert into iceberg.default.itable1 values (4);
select * from iceberg.default.mvtest1 ; I see the country corresponding to the nationkey
The freshness for |
1ba2559
to
2735195
Compare
(just rebased) |
Yes. In this case, there are two base tables
When you have a modification in the iceberg table, the MV is known to be stale, so the freshness is STALE. |
can this be related to a typo in the command:
? otherwise I couldn't reproduce this. |
2735195
to
da803c2
Compare
AC, thank you @findinpath for thorough review. |
The `listTables` callback takes schema name, so it should return table names without qualification. Allowing it to return `SchemaTableName` objects allows an implementation to incorrectly return tables with wrong schema name. For example, `TestEventListenerBasic.createQueryRunner` would create such bogus `listTables` implementation.
Require going through `anyTree()` by making `matchToAnyNodeTree` impl detail.
da803c2
to
e965274
Compare
We should probably update iceberg or MV docs to explain what REFRESH MV does when all the source tables are iceberg tables vs when there is a non-iceberg table involved. |
To a somewhat technical person, the previous wording would trigger questions that docs didn't not answer explicitly.
e965274
to
0f42396
Compare
@raunaqmorarka good point, updated the docs now. @mosabua i am going to merge this when build passes. if you have comments, i will happily apply them, but it's also ok to improve the wording as a follow-up. |
0f42396
to
ba0a796
Compare
So AWESOME @findepi |
Filed #15199 to follow up on the docs with some more clarifications. |
Indeed. It was apparently a typo. |
Probably fixes #13131