fix create view on exist table throw view exist exception by kyo-tom · Pull Request #10186 · trinodb/trino

kyo-tom · 2021-12-06T06:25:09Z

fix hive connector create view on exist table throw view exist exception. #10037
before:

trino:default> CREATE VIEW one AS SELECT * FROM one;
Query 20211206_045554_00007_33tp9 failed: View already exists: 'default.one'

after:

trino:default> CREATE VIEW one AS SELECT * FROM one;
Query 20211206_045554_00007_33tp9 failed: Table already exists: 'default.one'

Fixes #10186

lukasz-stec · 2021-12-08T08:01:48Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

IMO we should move this logic to the CreateViewTask and handle it in a similar way e.g. RenameViewTask handles it (including a check for materialized view). It would work then for all connectors (looks like iceberg connector may have the same issue) + would support MVs.

Yes, I also think it should work for all connectors, so the one that really needs to be enhanced is CreateViewTask. So I will resubmit a pr to make an enhancement to CreateViewTask, can I close this pr now?

I think it's ok to make changes in this PR (you can squash commits if you need to)

Ok i will do it in the next few days

cc @lukasz-stec , I am very confused about the function of ConnectorMetadata#getTableHandle in hive.it should return null when the relation is views (just like the comment says in this function). But if I create a view, it will not return null in hive because of tableActions will add a table when view create

yeah, hive connector implementation of ConnectorMetadata#getTableHandle breaks the contract. I don't think it's that easy to fix though (some features may depend on this behavior)

IMO we can first judge whether the view exists according to Metadata#isView, if the view does not exist, then judge whether the table exists, we also need to verify whether this is feasible on other connectors. Do you think we need open other issus about hive connector implementation of ConnectorMetadata#getTableHandle breaks the contract.

IMO we can first judge whether the view exists according to Metadata#isView

yes + metadata.isMaterializedView to check if its MV

Do you think we need open other issus about hive connector implementation of ConnectorMetadata#getTableHandle breaks the contract.

Good question. There is this ticket #8349 which is a kind of prerequisite for fixing the hive connector but maybe we need another one.

Hello, @lukasz-stec .Could you review my code again？ Thank you！

lukasz-stec · 2021-12-08T08:07:38Z

core/trino-main/src/test/java/io/trino/execution/TestCreateViewTask.java

the tests above are mostly checking this (mock) logic. If the checks are moved to CreateTableTask (see comment below) this will be unnecessary and you can extend the test class from BaseDataDefinitionTaskTest.

lukasz-stec · 2021-12-13T12:23:04Z

core/trino-main/src/test/java/io/trino/execution/TestCreateViewTask.java

How about using assertThat(metadata.isView(testSession, viewName)).isTrue(); instead of checcking getCreateViewCallCount?

lukasz-stec · 2021-12-13T12:24:00Z

core/trino-main/src/test/java/io/trino/execution/BaseDataDefinitionTaskTest.java

I would revert this and use metadata.isView instead

Yes,it can be revert if we use metadata.isView to check create view

lukasz-stec · 2021-12-13T12:32:18Z

core/trino-main/src/main/java/io/trino/execution/CreateViewTask.java

I think this make break for our case of hive connector returning tableHandle for views when replace=true

sorry, @lukasz-stec . I don't understand what you mean, , can you elaborate more？

Sorry, I should say may break...

So because getTableHandle will return not empty TableHandle for a view in hive connector, when statement.isReplace() == true (i.e. CREATE OR REPLACE VIEW was used) the method will trow
throw semanticException(TABLE_ALREADY_EXISTS, statement, "Table '%s' already exists", name); and it should replace the view definition successfuly.

Thank you for your reminder, I still need to check if it is view when statement.isReplace() == true.Sorry ,can you review again.

lukasz-stec

lgtm % comments

lukasz-stec · 2021-12-16T08:23:32Z

testing/trino-tests/src/test/java/io/trino/execution/TestEventListenerBasic.java

is this change needed?

Because the view named test_view is add to MockConnectorMetadata by withGetViews in TestEventListenerBasic, so metadata.isViews will always return true and throw a exception

Because the view named test_view is add to MockConnectorMetadata by withGetViews in TestEventListenerBasic, so metadata.isViews will always return true and throw a exception

Should use use different view name here then?

core/trino-main/src/main/java/io/trino/execution/CreateViewTask.java

sopel39 · 2021-12-17T10:05:06Z

core/trino-main/src/main/java/io/trino/execution/CreateViewTask.java

Change the order:

MV

view

table
This is the metadata check order that we use in other places. Let's be consistent here

@sopel39 I had changed the order,can you reviewer again,thank you

sopel39 · 2021-12-17T10:05:39Z

core/trino-main/src/main/java/io/trino/execution/CreateViewTask.java

else is not needed

sopel39 · 2021-12-17T10:05:59Z

core/trino-main/src/main/java/io/trino/execution/CreateViewTask.java

Check isView only after isMaterializedView (see other comment below)

core/trino-main/src/main/java/io/trino/execution/CreateViewTask.java

sopel39 · 2021-12-20T11:19:51Z

core/trino-main/src/main/java/io/trino/execution/CreateViewTask.java

use TABLE_ALREADY_EXISTS

sopel39 · 2021-12-20T11:19:56Z

core/trino-main/src/main/java/io/trino/execution/CreateViewTask.java

use TABLE_ALREADY_EXISTS

plugin/trino-memory/src/test/java/io/trino/plugin/memory/TestMemoryConnectorTest.java

sopel39 · 2021-12-20T11:23:56Z

testing/trino-tests/src/test/java/io/trino/execution/TestEventListenerBasic.java

Because the view named test_view is add to MockConnectorMetadata by withGetViews in TestEventListenerBasic, so metadata.isViews will always return true and throw a exception

Should use use different view name here then?

core/trino-main/src/test/java/io/trino/execution/TestCreateViewTask.java

plugin/trino-memory/src/test/java/io/trino/plugin/memory/TestMemoryConnectorTest.java

testing/trino-tests/src/test/java/io/trino/execution/TestEventListenerBasic.java

sopel39 · 2021-12-22T10:53:11Z

Fixes #10186

sopel39 · 2021-12-22T10:54:12Z

merged thanks!

cla-bot bot added the cla-signed label Dec 6, 2021

findepi requested a review from sopel39 December 6, 2021 13:06

sopel39 requested a review from lukasz-stec December 7, 2021 12:41

lukasz-stec reviewed Dec 8, 2021

View reviewed changes

kyo-tom force-pushed the create_view_message branch from 047fc0a to 8f8fb37 Compare December 13, 2021 11:36

kyo-tom requested a review from lukasz-stec December 13, 2021 11:45

lukasz-stec reviewed Dec 13, 2021

View reviewed changes

kyo-tom force-pushed the create_view_message branch 6 times, most recently from a546725 to 478045c Compare December 16, 2021 03:51

kyo-tom requested a review from lukasz-stec December 16, 2021 05:17

lukasz-stec approved these changes Dec 16, 2021

View reviewed changes

kyo-tom force-pushed the create_view_message branch 3 times, most recently from c5b4d9c to 7b98bd0 Compare December 17, 2021 02:15

sopel39 reviewed Dec 17, 2021

View reviewed changes

kyo-tom force-pushed the create_view_message branch from 7b98bd0 to 08d8b09 Compare December 18, 2021 07:09

sopel39 reviewed Dec 20, 2021

View reviewed changes

kyo-tom force-pushed the create_view_message branch 3 times, most recently from 50ffb45 to 5e3c356 Compare December 21, 2021 09:15

sopel39 approved these changes Dec 21, 2021

View reviewed changes

fix create view on exist table throw view exist exception

5897e63

kyo-tom force-pushed the create_view_message branch from 5e3c356 to 5897e63 Compare December 22, 2021 01:56

sopel39 merged commit 7053fb1 into trinodb:master Dec 22, 2021

sopel39 mentioned this pull request Dec 22, 2021

CREATE VIEW return incorrect information When a table with the same name exists In hive #10037

Open

sopel39 mentioned this pull request Dec 22, 2021

Release notes for 367 #10301

Closed

8 tasks

github-actions bot added this to the 367 milestone Dec 22, 2021

krvikash mentioned this pull request Jan 5, 2023

Correct exception type when table exists while creating a view #15580

Closed

Conversation

kyo-tom commented Dec 6, 2021 • edited by sopel39 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyo-tom Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukasz-stec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyo-tom Dec 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sopel39 commented Dec 22, 2021

Uh oh!

sopel39 commented Dec 22, 2021

Uh oh!

Reviewers

Assignees

kyo-tom commented Dec 6, 2021 •

edited by sopel39

Loading

kyo-tom Dec 16, 2021 •

edited

Loading

kyo-tom Dec 19, 2021 •

edited

Loading