Require that materialized view has owner #7489

sopel39 · 2021-04-02T15:38:24Z

Materialized views can only be analyzed with definer context.
Therefore view owner needs to be present.

findepi · 2021-04-07T12:54:34Z

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMaterializedViewDefinition.java

@@ -80,6 +80,10 @@ public ConnectorMaterializedViewDefinition(
        if (columns.isEmpty()) {
            throw new IllegalArgumentException("columns list is empty");
        }
+
+        if (owner.isEmpty()) {
+            throw new IllegalArgumentException("owner must be present");


what if we change the @JsonCreator ctor as well?
i think jackson-produced message would be good as well

what if we change the @JsonCreator ctor as well?

I didn't want to break backward compatibility of already serialized, correct materialized view definitions (with owner) that could have been stored somewhere by connector. Do you think it wouldn't be a problem?

you break it anyway -- if the serialized value doesn't have owner, it will fail during deserialization. It doesn't make a big difference whether it fails in jackson code vs in the constructor validtion as long as the raised message is informative.

It wouldn't fail if the serialized value does have owner. But if I change jackson constructor it would then fail then too. Technically, someone could already serialized such MV in metastore (we do something like that for ConnectorViewDefinition in Hive) and this change would break valid MV then.

It wouldn't fail if the serialized value does have owner. But if I change jackson constructor it would then fail then too.

I don't think this is the case (com.fasterxml.jackson.datatype.jdk8.Jdk8Module makes Optional<String> and String serialize the same way)

I'm pretty sure it's not the case. See #7318 (comment). Optional gets serialized into nested structure

We don't set com.fasterxml.jackson.datatype.jdk8.Jdk8Module#configureAbsentsAsNulls to true (default is false)

That doesn't align with my memory, but i'd need to check this out.

If so, we should definitely have a test exercising the persisted form build with a previous version against current code.
Especially if serialized form is an effect of interaction between SPI class (types, annotations) and a connector setup of ObjectMapper.

Ok It might work. Experiment:

ConnectorViewDefinition view = new ConnectorViewDefinition("foo", Optional.of("bar"), Optional.empty(), ImmutableList.of(new ConnectorViewDefinition.ViewColumn("f", TypeId.of("foo"))), Optional.empty(), Optional.empty(), false); String s = VIEW_CODEC.toJson(view);

{"originalSql":"foo","catalog":"bar","columns":[{"name":"f","type":"foo"}],"runAsInvoker":false}

findepi · 2021-04-07T12:55:09Z

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMaterializedViewDefinition.java

+
+        if (owner.isEmpty()) {
+            throw new IllegalArgumentException("owner must be present");
+        }


Also change the getter to be String getOwner

findepi · 2021-04-07T12:58:10Z

Materialized views can only be analyzed with definer context.

should StatementAnalyzer reflect this too?

sopel39 · 2021-04-07T13:00:03Z

should StatementAnalyzer reflect this too?

@findepi What do you mean?

findepi · 2021-04-07T13:31:08Z

should StatementAnalyzer reflect this too?

@findepi What do you mean?

this will be nothing to do if #7489 (comment)

sopel39

should StatementAnalyzer reflect this too?

statement analyzer cannot get empty now since MV constructor prevents that

sopel39 · 2021-04-09T11:19:50Z

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMaterializedViewDefinition.java

@@ -80,6 +80,10 @@ public ConnectorMaterializedViewDefinition(
        if (columns.isEmpty()) {
            throw new IllegalArgumentException("columns list is empty");
        }
+
+        if (owner.isEmpty()) {
+            throw new IllegalArgumentException("owner must be present");


Ok It might work. Experiment:

ConnectorViewDefinition view = new ConnectorViewDefinition("foo", Optional.of("bar"), Optional.empty(), ImmutableList.of(new ConnectorViewDefinition.ViewColumn("f", TypeId.of("foo"))), Optional.empty(), Optional.empty(), false); String s = VIEW_CODEC.toJson(view);

{"originalSql":"foo","catalog":"bar","columns":[{"name":"f","type":"foo"}],"runAsInvoker":false}

sopel39 · 2021-04-09T11:21:33Z

@findepi I would like to change JSON object in separate PR.

findepi · 2021-04-09T11:31:13Z

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMaterializedViewDefinition.java

@@ -44,10 +44,10 @@ public ConnectorMaterializedViewDefinition(
            @JsonProperty("schema") Optional<String> schema,
            @JsonProperty("columns") List<Column> columns,
            @JsonProperty("comment") Optional<String> comment,
-            @JsonProperty("owner") Optional<String> owner,
+            @JsonProperty("owner") String owner,


All these @JsonPropertys are misleading, since this is not a @JsonCreator.

added todo: #7537

findepi · 2021-04-09T11:32:28Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java

@@ -837,7 +837,7 @@ protected final void setup(String databaseName, HiveConfig hiveConfig, HiveMetas
                                Optional.empty(),
                                ImmutableList.of(new ConnectorMaterializedViewDefinition.Column("abc", TypeId.of("type"))),
                                Optional.empty(),
-                                Optional.empty(),
+                                "user",


is "user" a special value here?
"some mv owner"?

findepi · 2021-04-09T11:34:06Z

should StatementAnalyzer reflect this too?

statement analyzer cannot get empty now since MV constructor prevents that

yes, but this should be enforced in StatementAnalyzer, since it's its job.
And it still doesn't know that, since getOwner returns Optional

(will you change this in a followup?)

@findepi I would like to change JSON object in separate PR.

fine, please add a TODO for now

Materialized views can only be analyzed with definer context. Therefore view owner needs to be present.

sopel39

yes, but this should be enforced in StatementAnalyzer, since it's its job.
And it still doesn't know that, since getOwner returns Optional

done

fine, please add a TODO for now

#7537

sopel39 · 2021-04-09T11:58:29Z

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMaterializedViewDefinition.java

@@ -44,10 +44,10 @@ public ConnectorMaterializedViewDefinition(
            @JsonProperty("schema") Optional<String> schema,
            @JsonProperty("columns") List<Column> columns,
            @JsonProperty("comment") Optional<String> comment,
-            @JsonProperty("owner") Optional<String> owner,
+            @JsonProperty("owner") String owner,


added todo: #7537

sopel39 requested review from martint, findepi and anjalinorwood April 2, 2021 15:38

cla-bot bot added the cla-signed label Apr 2, 2021

anjalinorwood approved these changes Apr 6, 2021

View reviewed changes

sopel39 force-pushed the ks/mtv_owner branch 2 times, most recently from c585c56 to ccd3dba Compare April 7, 2021 12:50

findepi reviewed Apr 7, 2021

View reviewed changes

sopel39 commented Apr 9, 2021

View reviewed changes

findepi approved these changes Apr 9, 2021

View reviewed changes

Require that materialized view has owner

e90d92a

Materialized views can only be analyzed with definer context. Therefore view owner needs to be present.

sopel39 force-pushed the ks/mtv_owner branch from ccd3dba to e90d92a Compare April 9, 2021 11:58

sopel39 commented Apr 9, 2021

View reviewed changes

sopel39 merged commit 987b1dc into trinodb:master Apr 9, 2021

sopel39 deleted the ks/mtv_owner branch April 9, 2021 13:49

sopel39 mentioned this pull request Apr 9, 2021

Release notes for 356 #7533

Closed

9 tasks

martint added this to the 356 milestone Apr 26, 2021

m57lyra mentioned this pull request May 21, 2021

Document ConnectorMaterializedViewDefinition #8038

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require that materialized view has owner #7489

Require that materialized view has owner #7489

sopel39 commented Apr 2, 2021

findepi Apr 7, 2021

sopel39 Apr 7, 2021 •

edited

Loading

findepi Apr 7, 2021

sopel39 Apr 7, 2021

findepi Apr 7, 2021

sopel39 Apr 7, 2021

sopel39 Apr 7, 2021

findepi Apr 8, 2021

sopel39 Apr 9, 2021

findepi Apr 7, 2021

findepi commented Apr 7, 2021

sopel39 commented Apr 7, 2021

findepi commented Apr 7, 2021

sopel39 left a comment

sopel39 Apr 9, 2021

sopel39 commented Apr 9, 2021

findepi Apr 9, 2021

sopel39 Apr 9, 2021

findepi Apr 9, 2021

findepi commented Apr 9, 2021

sopel39 left a comment

sopel39 Apr 9, 2021

Require that materialized view has owner #7489

Require that materialized view has owner #7489

Conversation

sopel39 commented Apr 2, 2021

Choose a reason for hiding this comment

sopel39 Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Apr 7, 2021

sopel39 commented Apr 7, 2021

findepi commented Apr 7, 2021

sopel39 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 commented Apr 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Apr 9, 2021

sopel39 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 Apr 7, 2021 •

edited

Loading