Fix delete target table captured as input in event listener #16887
Conversation
|
@Praveen2112 I am going to add the test cases in the |
|
Yes and can we add a prep commit - which reproduces the issue (we could catch the failure) and then apply this commit to ensure that it works as expected. |
|
Sure, can, let me address it |
@Praveen2112 Added a test case to reproduce it, please review it. |
core/trino-main/src/main/java/io/trino/sql/planner/InputExtractor.java
Outdated
Show resolved
Hide resolved
testing/trino-tests/src/test/java/io/trino/execution/TestEventListenerBasic.java
Outdated
Show resolved
Hide resolved
| private void assertIoInputTable(String sql, Set<String> inputTables) | ||
| throws Exception | ||
| { | ||
| QueryEvents queryEvents = runQueryAndWaitForEvents(sql).getQueryEvents(); | ||
| QueryCompletedEvent event = queryEvents.getQueryCompletedEvent(); | ||
| assertThat(event.getIoMetadata().getInputs()) | ||
| .map(TestEventListenerBasic::getQualifiedName) | ||
| .containsExactlyInAnyOrderElementsOf(inputTables); |
There was a problem hiding this comment.
Can we use assertLineageInternal instead of this new API ?
There was a problem hiding this comment.
Note the Line 1237 we are verify the event.getIoMetadata().getInputs() and the view creation won't have any input. Could you suggest how to use assertLineageInternal to verify?
There was a problem hiding this comment.
Can you elaborate more here ? We consider the query and run other operations on top of it.
There was a problem hiding this comment.
Why don't we analyze event.getMetadata().getTables()
There was a problem hiding this comment.
QueryInputMetadata returns almost all the input tables that are processed by a query and if the query has views - the view information is lost in QueryInputMetadata - so we treat QueryMetadata#getTables as a source of truth for lineage details.
There was a problem hiding this comment.
Why don't we
analyze event.getMetadata().getTables()
The analyze event.getMetadata().getTables() is different from the event.getIoMetadata().getInputs(), and the pr is for fixing the delete table occurs in the event.getIoMetadata().getInputs() see #16433
testing/trino-tests/src/test/java/io/trino/execution/TestEventListenerBasic.java
Outdated
Show resolved
Hide resolved
| private void assertIoInputTable(String sql, Set<String> inputTables) | ||
| throws Exception | ||
| { | ||
| QueryEvents queryEvents = runQueryAndWaitForEvents(sql).getQueryEvents(); | ||
| QueryCompletedEvent event = queryEvents.getQueryCompletedEvent(); | ||
| assertThat(event.getIoMetadata().getInputs()) | ||
| .map(TestEventListenerBasic::getQualifiedName) | ||
| .containsExactlyInAnyOrderElementsOf(inputTables); |
There was a problem hiding this comment.
Can you elaborate more here ? We consider the query and run other operations on top of it.
| private void assertIoInputTable(String sql, Set<String> inputTables) | ||
| throws Exception | ||
| { | ||
| QueryEvents queryEvents = runQueryAndWaitForEvents(sql).getQueryEvents(); | ||
| QueryCompletedEvent event = queryEvents.getQueryCompletedEvent(); | ||
| assertThat(event.getIoMetadata().getInputs()) | ||
| .map(TestEventListenerBasic::getQualifiedName) | ||
| .containsExactlyInAnyOrderElementsOf(inputTables); |
There was a problem hiding this comment.
Why don't we analyze event.getMetadata().getTables()
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
👋 @chenjian2664 @Praveen2112 - this PR has become inactive. We hope you are still interested in working on it. Please let us know, and we can try to get reviewers to help with that. We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks. |
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
Description
Fix #16433
Additional context and related issues
Release notes
(x) Release notes are required, with the following suggested text: