Skip to content

Utilized Columns fix for views #24638

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
Merged
kevintang2022 merged 1 commit into from
Mar 28, 2025
Merged

Utilized Columns fix for views #24638

kevintang2022 merged 1 commit into from
Mar 28, 2025

Conversation

@kevintang2022
Copy link
Contributor

@kevintang2022 kevintang2022 commented Feb 26, 2025 •
edited
Loading

Description

  • Perform the exploration of a view's definition just like how we would perform exploration of a subquery

Motivation and Context

T214103298
The user's dataswarm tasks is failing because the ACL is checking every column in the view table when it should only be checking ACL on the utilized columns

Impact

Test Plan

Added automated tests for queries involving views

Deploy to verifier cluster

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

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Improve analysis of utilized columns in a query by exploring view definitions and checking the utilized columns of the underlying tables

@kevintang2022 kevintang2022 force-pushed the utlized-columns-fix-for-views branch from 31a3208 to 682f652 Compare February 27, 2025 17:03
@kevintang2022 kevintang2022 changed the title WIP Utilized Columns fix for views Feb 27, 2025
@kevintang2022 kevintang2022 force-pushed the utlized-columns-fix-for-views branch from fd94abe to 6423cf4 Compare February 27, 2025 17:08
@kevintang2022 kevintang2022 force-pushed the utlized-columns-fix-for-views branch from a998a45 to 0ca3ede Compare March 19, 2025 04:01
@kevintang2022 kevintang2022 marked this pull request as ready for review March 19, 2025 20:33
rschlussel
rschlussel reviewed Mar 20, 2025
View reviewed changes
presto-main/src/test/java/com/facebook/presto/sql/analyzer/AbstractAnalyzerTest.java Outdated
ImmutableList.of(new ColumnMetadata("a", BIGINT)));
inSetupTransaction(session -> metadata.createView(session, TPCH_CATALOG, viewMetadata5, viewData5, false));

String viewData6 = JsonCodec.jsonCodec(ViewDefinition.class).toJson(
Copy link
Contributor

@rschlussel rschlussel Mar 19, 2025

Choose a reason for hiding this comment

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

i know everything else just uses t, v, etc. but can you give these views more descriptive names so it's easier to understand the tests that use them?

presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestUtilizedColumnsAnalyzer.java Outdated
Statement statement = SQL_PARSER.createStatement(query);
Analysis analysis = analyzer.analyze(statement);
assertEquals(analysis.getUtilizedTableColumnReferences().values().stream().findFirst().get(), expected);
assertEquals(analysis.getUtilizedTableColumnReferences().values(), expected);
Copy link
Contributor

@rschlussel rschlussel Mar 19, 2025

Choose a reason for hiding this comment

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

can you assert on the whole map (incl. access control info) and not just the values? that way we can be sure it's all correct for definer vs. invoker.

@steveburnett
Copy link
Contributor

steveburnett commented Mar 20, 2025

Thanks for the release note! Suggest rephrasing to follow the Order of Changes in the Release Notes Guidelines. I adapted phrasing from your Description in the PR, but if you have a better way to phrase it, please do.

== RELEASE NOTES ==

General Changes
* Improve exploration of a view's definition by checking ACL only on the utilized columns.

rschlussel
rschlussel reviewed Mar 26, 2025
View reviewed changes
presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestUtilizedColumnsAnalyzer.java Outdated

assertUtilizedTableColumns(query, ImmutableSet.of(
assertUtilizedTableColumns(query,
ImmutableMap.of(
Copy link
Contributor

@rschlussel rschlussel Mar 26, 2025

Choose a reason for hiding this comment

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

shouldn't definer mode expect 2 different accessControlInfos?

Copy link
Contributor Author

@kevintang2022 kevintang2022 Mar 26, 2025

Choose a reason for hiding this comment

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

When I ran this, there were 3 access control infos.
AccessControl1: [view_definer1, view_definer2]
AccessControl2: [t1]
AccessControl3: [t13]

Copy link
Contributor Author

@kevintang2022 kevintang2022 Mar 26, 2025

Choose a reason for hiding this comment

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

I recently changed the test case because I added an equals method to AccessControlContext

Copy link
Contributor Author

@kevintang2022 kevintang2022 Mar 26, 2025

Choose a reason for hiding this comment

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

For this query, can you give me what the expected result should actually be?

Copy link
Contributor Author

@kevintang2022 kevintang2022 Mar 26, 2025

Choose a reason for hiding this comment

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

It seems we create a new accessControl when it's definer mode, but not invoker mode

                if (owner.isPresent() && !owner.get().equals(session.getIdentity().getUser())) {
                    // definer mode
                    identity = new Identity(owner.get(), Optional.empty(), session.getIdentity().getExtraCredentials());
                    viewAccessControl = new ViewAccessControl(accessControl);
                }
                else {
                    identity = session.getIdentity();
                    viewAccessControl = accessControl;
                }

@kevintang2022 kevintang2022 force-pushed the utlized-columns-fix-for-views branch from 4fb46b7 to 643340d Compare March 26, 2025 19:31
kevintang2022
kevintang2022 commented Mar 26, 2025
View reviewed changes
presto-spi/src/main/java/com/facebook/presto/spi/security/AccessControlContext.java Outdated
Objects.equals(this.clientInfo, other.clientInfo) &&
Objects.equals(this.clientTags, other.clientTags) &&
Objects.equals(this.source, other.source) &&
Objects.equals(this.queryType, other.queryType);
Copy link
Contributor Author

@kevintang2022 kevintang2022 Mar 26, 2025

Choose a reason for hiding this comment

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

Also need to include schema

@kevintang2022 kevintang2022 force-pushed the utlized-columns-fix-for-views branch from 7aa8b31 to 524ba57 Compare March 26, 2025 20:56
rschlussel
rschlussel previously approved these changes Mar 26, 2025
View reviewed changes
@kevintang2022 kevintang2022 force-pushed the utlized-columns-fix-for-views branch 3 times, most recently from 2f28152 to da2ea23 Compare March 27, 2025 16:34
rschlussel
rschlussel previously approved these changes Mar 27, 2025
View reviewed changes
@kevintang2022
Utilized columns fix for views
d74935c 
Update test file to check all utilized references

Add test for definer mode

Lint

Untab

Untab 2

Remove line break

Lint

Lint

Add cte to view definition

Lint

Fix naming of test tables

Check access control info in tests

Remove wrapper set

Check ViewAccessControlEquality

Refactor test

Lint

Fix build

Update equals method

Add check to catalog

Add catalog to hashcode

Merge unnest
@kevintang2022 kevintang2022 force-pushed the utlized-columns-fix-for-views branch from da2ea23 to d74935c Compare March 27, 2025 20:51
@kevintang2022
Copy link
Contributor Author

kevintang2022 commented Mar 27, 2025

Needed to fix merge conflict on the test file

@kevintang2022 kevintang2022 merged commit 4eb4ce2 into prestodb:master Mar 28, 2025
96 checks passed
@prestodb-ci prestodb-ci mentioned this pull request Mar 28, 2025
Merged
30 tasks
pradeepvaka pushed a commit to pradeepvaka/presto that referenced this pull request Apr 11, 2025
@kevintang2022 @pradeepvaka
Utilized columns fix for views (prestodb#24638)
e9cea0e 
Update test file to check all utilized references

Add test for definer mode

Lint

Untab

Untab 2

Remove line break

Lint

Lint

Add cte to view definition

Lint

Fix naming of test tables

Check access control info in tests

Remove wrapper set

Check ViewAccessControlEquality

Refactor test

Lint

Fix build

Update equals method

Add check to catalog

Add catalog to hashcode

Merge unnest
@unidevel unidevel mentioned this pull request Apr 25, 2025
Closed
30 tasks
This was referenced May 6, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rschlussel rschlussel rschlussel approved these changes

@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

@vivek-bharathan vivek-bharathan Awaiting requested review from vivek-bharathan

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kevintang2022 @steveburnett @rschlussel