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.

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

Jump to bottom

Support Iceberg materialized views with types not supported directly in Iceberg library #16050

Merged
merged 4 commits into from
Mar 9, 2023
Merged

Support Iceberg materialized views with types not supported directly in Iceberg library #16050

merged 4 commits into from
Mar 9, 2023

Conversation

findepi
Copy link
Member

@findepi findepi commented Feb 9, 2023

No description provided.

@cla-bot cla-bot bot added the cla-signed label Feb 9, 2023
@findepi findepi force-pushed the findepi/mv-flex-type branch from 39eb0a7 to e4a8319 Compare February 9, 2023 20:00
@findepi findepi requested a review from raunaqmorarka March 2, 2023 21:48
@findepi findepi force-pushed the findepi/mv-flex-type branch from e4a8319 to bc224dc Compare March 3, 2023 10:47
@findepi
Copy link
Member Author

findepi commented Mar 3, 2023

(just rebased)

@github-actions github-actions bot added the iceberg Iceberg connector label Mar 3, 2023
@findepi
Support arbitrary types in Iceberg materialized view definition
ac24bbf 
A materialized view should be able to capture any query results and
these are not constrained to be limited to types directly supported by
Iceberg.

Coerce unsupported types to supported ones.

When reading, when the storage table has types different than MV output
types, coercions are already applied by the `StatementAnalyzer`.

The MV/storage schema mismatch is not supported in REFRESH MATERIALIZED
VIEW yet.
@findepi findepi force-pushed the findepi/mv-flex-type branch from bc224dc to bdf6982 Compare March 3, 2023 12:15
@findepi findepi requested a review from sopel39 March 3, 2023 12:16
electrum
electrum previously requested changes Mar 8, 2023
View reviewed changes
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.

How is the data for an arbitrary type stored? We don’t have any way for a type to declare its stable representation or how it should be serialized/deserialized. I looked at the code but couldn’t find where this happens.

@findepi
Copy link
Member Author

findepi commented Mar 8, 2023

the solution does not support arbitrary types. only the chosen, selected, types from among standard type.

@findepi findepi dismissed electrum’s stale review March 8, 2023 13:22

question answered

@findepi
Support REFRESH MATERIALIZED VIEW when storage differs in schema
b5e0c23 
The materialized view's storage table may have a schema different than
the materialized view itself. This is already supported when reading
(the coercions are getting applied as necessary), but was not supported
for REFRESH. In the REFRESH normal INSERT constraints where getting
applied, thus not allowing the materialized view to, for example, store
temporal values in a varchar column.

The change removes the constraint, and therefore a storage table may use
any type that is coercible to from the view definition type. Of course,
the implementations should only use types where coercion to the storage
and backwards does round trip.
@findepi findepi force-pushed the findepi/mv-flex-type branch from bdf6982 to b5e0c23 Compare March 8, 2023 15:10
@findepi
Copy link
Member Author

findepi commented Mar 9, 2023

CI unrelated

Error:  io.trino.plugin.sqlserver.TestSqlServerWithSnapshotIsolation.init  Time elapsed: 0.449 s  <<< FAILURE!
org.testcontainers.containers.ContainerLaunchException: Container startup failed
	at org.testcontainers.containers.GenericContainer.doStart(GenericContainer.java:349)
	at org.testcontainers.containers.GenericContainer.start(GenericContainer.java:322)
	at io.trino.testing.containers.TestContainers.startOrReuse(TestContainers.java:48)
	at io.trino.plugin.sqlserver.TestingSqlServer.createContainer(TestingSqlServer.java:144)
	at io.trino.plugin.sqlserver.TestingSqlServer.lambda$new$3(TestingSqlServer.java:92)
	at dev.failsafe.Functions.lambda$toCtxSupplier$11(Functions.java:243)
	at dev.failsafe.Functions.lambda$get$0(Functions.java:46)
	at dev.failsafe.internal.TimeoutExecutor.lambda$apply$1(TimeoutExecutor.java:93)
	at dev.failsafe.internal.RetryPolicyExecutor.lambda$apply$0(RetryPolicyExecutor.java:74)
	at dev.failsafe.SyncExecutionImpl.executeSync(SyncExecutionImpl.java:187)
	at dev.failsafe.FailsafeExecutor.call(FailsafeExecutor.java:376)
	at dev.failsafe.FailsafeExecutor.get(FailsafeExecutor.java:112)
	at io.trino.plugin.sqlserver.TestingSqlServer.<init>(TestingSqlServer.java:92)
	at io.trino.plugin.sqlserver.TestingSqlServer.<init>(TestingSqlServer.java:86)
	at io.trino.plugin.sqlserver.BaseSqlServerTransactionIsolationTest.createQueryRunner(BaseSqlServerTransactionIsolationTest.java:37)
	at io.trino.testing.AbstractTestQueryFramework.init(AbstractTestQueryFramework.java:105)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
	at org.testng.internal.Invoker.invokeConfigurationMethod(Invoker.java:515)
	at org.testng.internal.Invoker.invokeConfigurations(Invoker.java:217)
	at org.testng.internal.Invoker.invokeConfigurations(Invoker.java:144)
	at org.testng.internal.TestMethodWorker.invokeBeforeClassMethods(TestMethodWorker.java:169)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:108)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: org.rnorth.ducttape.RetryCountExceededException: Retry limit hit with exception
	at org.rnorth.ducttape.unreliables.Unreliables.retryUntilSuccess(Unreliables.java:88)
	at org.testcontainers.containers.GenericContainer.doStart(GenericContainer.java:334)
	... 28 more
Caused by: org.testcontainers.containers.ContainerLaunchException: Could not create/start container
	at org.testcontainers.containers.GenericContainer.tryStart(GenericContainer.java:542)
	at org.testcontainers.containers.GenericContainer.lambda$doStart$0(GenericContainer.java:344)
	at org.rnorth.ducttape.unreliables.Unreliables.retryUntilSuccess(Unreliables.java:81)
	... 29 more
Caused by: java.lang.IllegalStateException: Container exited with code 1
	at org.testcontainers.containers.GenericContainer.tryStart(GenericContainer.java:514)
	... 31 more

@findepi findepi merged commit 11942a4 into trinodb:master Mar 9, 2023
@findepi findepi deleted the findepi/mv-flex-type branch March 9, 2023 14:26
@github-actions github-actions bot added this to the 411 milestone Mar 9, 2023
@colebow colebow mentioned this pull request Mar 14, 2023
Merged
@colebow
Copy link
Member

colebow commented Mar 14, 2023

@findepi does this need release notes?

@findepi
Copy link
Member Author

findepi commented Mar 15, 2023

@colebow yes.
maybe something like "Support Iceberg materialized views with types not supported directly in Iceberg library", ie the PR title, would work?

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

@sopel39 sopel39 sopel39 approved these changes

@electrum electrum electrum left review comments

@homar homar Awaiting requested review from homar

@alexjo2144 alexjo2144 Awaiting requested review from alexjo2144

@anjalinorwood anjalinorwood Awaiting requested review from anjalinorwood

@findinpath findinpath Awaiting requested review from findinpath

@raunaqmorarka raunaqmorarka Awaiting requested review from raunaqmorarka

Assignees
No one assigned
Labels
cla-signed iceberg Iceberg connector
Milestone
411
Development

Successfully merging this pull request may close these issues.
5 participants
@findepi @colebow @electrum @raunaqmorarka @sopel39