-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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.
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
Add option to do more precise ACL checks #15269
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First set of comments.
presto-main/src/main/java/com/facebook/presto/sql/analyzer/Analyzer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/Analysis.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/Analyzer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/PostPruningTableColumnsAnalyzer.java
Outdated
Show resolved
Hide resolved
Will be good to have more expression coverage - look at the g4 file and add all non-standard places that can have expressions - patterns like lambdas, using, grouping sets, order by of aggs etc. |
Also update the title to say make the ACL checks more precise to include any and all that impact the final output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test failures seem to be related?
presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java
Outdated
Show resolved
Hide resolved
cbd8b0f
to
bbc4184
Compare
f641d38
to
b8e6165
Compare
presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java
Outdated
Show resolved
Hide resolved
e0b6bf9
to
a600db5
Compare
a600db5
to
9fe4d50
Compare
Done, added a bunch of tests to cover expressions
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extensive set of tests! A few more widely used cases missing that you should add those.
presto-main/src/main/java/com/facebook/presto/sql/analyzer/Analyzer.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestUtilizedColumnsAnalyzer.java
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestUtilizedColumnsAnalyzer.java
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestUtilizedColumnsAnalyzer.java
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestUtilizedColumnsAnalyzer.java
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestUtilizedColumnsAnalyzer.java
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestUtilizedColumnsAnalyzer.java
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestUtilizedColumnsAnalyzer.java
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestUtilizedColumnsAnalyzer.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add window functions - both in aggs as well as partition by and order by without referring to them in the select list.
I have a testWindowFunction which covers column references in a PARTITION BY and ORDER BY clause in a window function |
+1 |
9fe4d50
to
5e75f09
Compare
presto-main/src/main/java/com/facebook/presto/sql/analyzer/Analysis.java
Show resolved
Hide resolved
5e75f09
to
e0f2a8c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just add one more test for struct/row types - I know we do only top level structs but good to have coverage. Similarly that are potentially short circuited like:
IF(FALSE, x, y) -> ACL both x and y
WHERE x > 0 AND false AND y < 10 -> ACL both x,y
e0f2a8c
to
3e5e74d
Compare
I have test for ROW in testConstructors, and I added tests covering these short-circuit cases. |
Before this change, Presto would check for column access permission on all columns referenced in any part of the query. This behavior can sometimes be undesirable, for example, in this query: ``SELECT name FROM (SELECT * FROM nation)`` During execution of this query, access checks would be performed on all columns in the table nation, even though only the column ``name`` would actually be read during the execution of the query, and the other columns in the table have no impact on the query results. This change introduces a new sesion property, ``check_access_control_on_utilized_columns_only``, which, when enabled, will only perform access control checks on columns that would actually be required to produce the query output, ignoring columns that are referenced in the query, but are not required to compute the query results.
3e5e74d
to
29467d8
Compare
Before this change, Presto would check for column access permission on
all columns referenced in any part of the query.
This behavior can sometimes be undesirable, for example, in this query:
SELECT name FROM (SELECT * FROM nation)
During execution of this query, access checks would be performed on all
columns in the table nation, even though only the column
name
wouldactually be read during the execution of the query, and the other
columns in the table have no impact on the query results.
This change introduces a new sesion property,
check_access_control_on_utilized_columns_only
, which, whenenabled, will only perform access control checks on columns that would
actually be required to produce the query output, ignoring columns that
are referenced in the query, but are not required to compute the query
results.
Test plan -