Skip to content

Enrich Update information in QueryInfo #24899

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Enrich Update information in QueryInfo #24899

wants to merge 1 commit into from

Conversation

evanvdia
Copy link
Contributor

@evanvdia evanvdia commented Apr 10, 2025 •
edited by aaneja
Loading

Description

Add additional information to about DDL operations to QueryInfo

Motivation and Context

Issue: #24894

updateType tracking in QueryInfo was added in 3715be8

For lineage tracing, we would like to enhance this with additional information about the object(s) we are updating

Impact

User's can track what tables/columns/schemas were operated on in query info and event listener data

Test Plan

CI

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

== NO RELEASE NOTE ==

@evanvdia evanvdia requested a review from presto-oss April 10, 2025 09:20
@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Apr 10, 2025
@prestodb-ci prestodb-ci requested review from a team, nmahadevuni and ShahimSharafudeen and removed request for a team April 10, 2025 09:20
@aaneja aaneja mentioned this pull request Apr 17, 2025
Closed
6 tasks
aaneja
aaneja requested changes Apr 17, 2025
View reviewed changes
Copy link
Contributor

@aaneja aaneja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation you have is limited in scope to specific table Statement types. We also have an existing field in QueryInfo called updateType for tracing the type of update statement that was made.

IMO we should make changes to start tracing more information in a POJO that we attach during query analysis. I've made a (draft) PR for this - #24933

Please take a look, and feel free to productionize it

@evanvdia
Copy link
Contributor Author

@aaneja Will check and implement these changes.

@evanvdia evanvdia force-pushed the qualified_name_for_alter_and_drop branch 3 times, most recently from 831efc0 to 9a3f573 Compare April 24, 2025 19:27
@evanvdia
Copy link
Contributor Author

@aaneja I have incorporated the changes from #24933.

@evanvdia evanvdia requested a review from aaneja April 25, 2025 03:48
@evanvdia evanvdia force-pushed the qualified_name_for_alter_and_drop branch 3 times, most recently from e239b0a to 3a05f11 Compare April 29, 2025 03:33
@ethanyzhang
Copy link
Contributor

Hi @evanvdia can you rebase?

@evanvdia evanvdia force-pushed the qualified_name_for_alter_and_drop branch from 3a05f11 to c85c0fe Compare May 2, 2025 09:30
@evanvdia evanvdia changed the title Set table qualified name for alter and drop operations at query events Enrich Update information in QueryInfo May 7, 2025
aaneja
aaneja reviewed May 7, 2025
View reviewed changes
presto-main-base/src/main/java/com/facebook/presto/execution/DDLDefinitionExecution.java
AnalyzerContext analyzerContext = getAnalyzerContext(queryAnalyzer,
metadata.getMetadataResolver(stateMachine.getSession()), new PlanNodeIdAllocator(), new VariableAllocator(), stateMachine.getSession(), query);
QueryAnalysis analysis = queryAnalyzer.analyze(analyzerContext, preparedQuery);
stateMachine.setUpdateInfo(analysis.getUpdateInfo());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core of the change is here - we now run DDL statements through the analyzer and use analysis.getUpdateInfo() to set the update info
This responsibility to set the correct/apt info about the DDL operation will now rest solely on StatementAnalyzer

@evanvdia evanvdia force-pushed the qualified_name_for_alter_and_drop branch from c85c0fe to 7fdebdd Compare May 7, 2025 06:36
aaneja
aaneja reviewed May 7, 2025
View reviewed changes
presto-spi/src/main/java/com/facebook/presto/spi/analyzer/UpdateInfo.java
public class UpdateInfo
{
private final String updateType;
private final String updateObject;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DDL statements can stringify what is relevant to them to track and store it in this updateObject

@evanvdia evanvdia force-pushed the qualified_name_for_alter_and_drop branch from 7fdebdd to 40283b9 Compare May 7, 2025 17:22
@evanvdia evanvdia force-pushed the qualified_name_for_alter_and_drop branch from 40283b9 to 37fc1c3 Compare May 8, 2025 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaneja aaneja aaneja requested changes

@shrinidhijoshi shrinidhijoshi Awaiting requested review from shrinidhijoshi shrinidhijoshi is a code owner

@jaystarshot jaystarshot Awaiting requested review from jaystarshot jaystarshot is a code owner

@feilong-liu feilong-liu Awaiting requested review from feilong-liu feilong-liu is a code owner

@elharo elharo Awaiting requested review from elharo elharo is a code owner

@ClarenceThreepwood ClarenceThreepwood Awaiting requested review from ClarenceThreepwood ClarenceThreepwood is a code owner

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

@nmahadevuni nmahadevuni Awaiting requested review from nmahadevuni nmahadevuni was automatically assigned from prestodb/ibm-reviewers

@ShahimSharafudeen ShahimSharafudeen Awaiting requested review from ShahimSharafudeen ShahimSharafudeen was automatically assigned from prestodb/ibm-reviewers

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
from:IBM PR from IBM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@evanvdia @ethanyzhang @aaneja @prestodb-ci