Skip to content

Commit 2ae4e02

Browse files
kevintang2022kevintang2022
authored
fix: Add global tracker for view definitions used in a query (prestodb#26927)
## Description Currently, view definitions are tracked inside of `Analysis` class, but it's possible that a new `Analysis` instance gets generated each time a statement rewrite occurs (for example explain rewrite). However, for permissions checks to be comprehensive, a full map of every view definition needs to be collected on the query level. This PR adds a variable `viewDefinitionReferences` that keep track of the view definitions used at the query level. During statement analysis, any view that is visited will be added to the tracker. ## Motivation and Context Currently, with statement rewrites, like explain rewrite, it is challenging to have a full picture of every view definition used during query analysis. Adding this `viewDefinitionReferences` collector will produce a global view of every view definition in a query. ## Impact - Pass `viewDefinitionReferences` from Analyzer down to StatementAnalyzer so that the `Analysis` tracking all view definitions in the global query scope - Clean up unnecessary view definition and `queryAccessControlInfo` logic in `AccessControlReferences` ## Test Plan Add unit test cases for checkQueryIntegrity and ensure that each shape runs the check at least once. Also added unit test to ensure view definitions can be read from the analysis after analyze. No impact on query execution. Just moving the view definition fields from one class to another. Internal testing: [P2139832088](https://www.internalfb.com/phabricator/paste/view/P2139832088) ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#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](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. - [ ] If adding new dependencies, verified they have an [OpenSSF Scorecard](https://securityscorecards.dev/#the-checks) score of 5.0 or higher (or obtained explicit TSC approval for lower scores). ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == NO RELEASE NOTE == ``` Differential Revision: D90355103
1 parent 3d8af97 commit 2ae4e02

35 files changed

+505
-147
lines changed

‎presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/Analysis.java‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import com.facebook.presto.spi.analyzer.AccessControlReferences;
2929
import com.facebook.presto.spi.analyzer.AccessControlRole;
3030
import com.facebook.presto.spi.analyzer.UpdateInfo;
31+
import com.facebook.presto.spi.analyzer.ViewDefinitionReferences;
3132
import com.facebook.presto.spi.connector.ConnectorTransactionHandle;
3233
import com.facebook.presto.spi.eventlistener.OutputColumnMetadata;
3334
import com.facebook.presto.spi.function.FunctionHandle;
@@ -241,11 +242,14 @@ public class Analysis
241242
// Row id field used for MERGE INTO command.
242243
private final Map<NodeRef<Table>, FieldReference> rowIdField = new LinkedHashMap<>();
243244

244-
public Analysis(@Nullable Statement root, Map<NodeRef<Parameter>, Expression> parameters, boolean isDescribe)
245+
private final ViewDefinitionReferences viewDefinitionReferences;
246+
247+
public Analysis(@Nullable Statement root, Map<NodeRef<Parameter>, Expression> parameters, boolean isDescribe, ViewDefinitionReferences viewDefinitionReferences)
245248
{
246249
this.root = root;
247250
this.parameters = ImmutableMap.copyOf(requireNonNull(parameters, "parameterMap is null"));
248251
this.isDescribe = isDescribe;
252+
this.viewDefinitionReferences = requireNonNull(viewDefinitionReferences, "viewDefinitionReferences is null");
249253
}
250254

251255
public Statement getStatement()
@@ -957,9 +961,9 @@ public AccessControlReferences getAccessControlReferences()
957961
return accessControlReferences;
958962
}
959963

960-
public void addQueryAccessControlInfo(AccessControlInfo accessControlInfo)
964+
public ViewDefinitionReferences getViewDefinitionReferences()
961965
{
962-
accessControlReferences.setQueryAccessControlInfo(accessControlInfo);
966+
return viewDefinitionReferences;
963967
}
964968

965969
public void addAccessControlCheckForTable(AccessControlRole accessControlRole, AccessControlInfoForTable accessControlInfoForTable)

‎presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/BuiltInQueryAnalysis.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import com.facebook.presto.spi.analyzer.AccessControlReferences;
1919
import com.facebook.presto.spi.analyzer.QueryAnalysis;
2020
import com.facebook.presto.spi.analyzer.UpdateInfo;
21+
import com.facebook.presto.spi.analyzer.ViewDefinitionReferences;
2122
import com.facebook.presto.spi.function.FunctionKind;
2223
import com.facebook.presto.sql.tree.Explain;
2324
import com.google.common.collect.ImmutableSet;
@@ -65,6 +66,12 @@ public AccessControlReferences getAccessControlReferences()
6566
return analysis.getAccessControlReferences();
6667
}
6768

69+
@Override
70+
public ViewDefinitionReferences getViewDefinitionReferences()
71+
{
72+
return analysis.getViewDefinitionReferences();
73+
}
74+
6875
@Override
6976
public boolean isExplainAnalyzeQuery()
7077
{

‎presto-main-base/src/main/java/com/facebook/presto/execution/AccessControlCheckerExecution.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ private ListenableFuture<?> executeTask()
271271
}
272272

273273
stateMachine.beginColumnAccessPermissionChecking();
274-
checkAccessPermissions(queryAnalysis.getAccessControlReferences(), query, getSession().getPreparedStatements());
274+
checkAccessPermissions(queryAnalysis.getAccessControlReferences(), queryAnalysis.getViewDefinitionReferences(), query, getSession().getPreparedStatements(), getSession().getIdentity(), accessControl, getSession().getAccessControlContext());
275275
stateMachine.endColumnAccessPermissionChecking();
276276
return immediateFuture(null);
277277
}

‎presto-main-base/src/main/java/com/facebook/presto/execution/AlterFunctionTask.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import com.facebook.presto.spi.MaterializedViewDefinition;
2121
import com.facebook.presto.spi.WarningCollector;
2222
import com.facebook.presto.spi.analyzer.ViewDefinition;
23+
import com.facebook.presto.spi.analyzer.ViewDefinitionReferences;
2324
import com.facebook.presto.spi.function.AlterRoutineCharacteristics;
2425
import com.facebook.presto.spi.function.RoutineCharacteristics.NullCallClause;
2526
import com.facebook.presto.spi.security.AccessControl;
@@ -73,9 +74,9 @@ public String explain(AlterFunction statement, List<Expression> parameters)
7374
public ListenableFuture<?> execute(AlterFunction statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, Session session, List<Expression> parameters, WarningCollector warningCollector, String query)
7475
{
7576
Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
76-
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, warningCollector, query);
77+
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, warningCollector, query, new ViewDefinitionReferences());
7778
Analysis analysis = analyzer.analyzeSemantic(statement, false);
78-
checkAccessPermissions(analysis.getAccessControlReferences(), query, session.getPreparedStatements());
79+
checkAccessPermissions(analysis.getAccessControlReferences(), analysis.getViewDefinitionReferences(), query, session.getPreparedStatements(), session.getIdentity(), accessControl, session.getAccessControlContext());
7980

8081
QualifiedObjectName functionName = metadata.getFunctionAndTypeManager().getFunctionAndTypeResolver().qualifyObjectName(statement.getFunctionName());
8182
AlterRoutineCharacteristics alterRoutineCharacteristics = new AlterRoutineCharacteristics(

‎presto-main-base/src/main/java/com/facebook/presto/execution/CreateFunctionTask.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import com.facebook.presto.spi.MaterializedViewDefinition;
2222
import com.facebook.presto.spi.PrestoException;
2323
import com.facebook.presto.spi.analyzer.ViewDefinition;
24+
import com.facebook.presto.spi.analyzer.ViewDefinitionReferences;
2425
import com.facebook.presto.spi.function.Parameter;
2526
import com.facebook.presto.spi.function.RoutineCharacteristics;
2627
import com.facebook.presto.spi.function.SqlFunctionHandle;
@@ -88,9 +89,9 @@ public ListenableFuture<?> execute(CreateFunction statement, TransactionManager
8889
{
8990
Map<NodeRef<com.facebook.presto.sql.tree.Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
9091
Session session = stateMachine.getSession();
91-
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, stateMachine.getWarningCollector(), query);
92+
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, stateMachine.getWarningCollector(), query, new ViewDefinitionReferences());
9293
Analysis analysis = analyzer.analyzeSemantic(statement, false);
93-
checkAccessPermissions(analysis.getAccessControlReferences(), query, session.getPreparedStatements());
94+
checkAccessPermissions(analysis.getAccessControlReferences(), analysis.getViewDefinitionReferences(), query, session.getPreparedStatements(), session.getIdentity(), accessControl, session.getAccessControlContext());
9495

9596
if (analysis.getFunctionHandles().values().stream()
9697
.anyMatch(SqlFunctionHandle.class::isInstance)) {

‎presto-main-base/src/main/java/com/facebook/presto/execution/CreateMaterializedViewTask.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import com.facebook.presto.spi.SchemaTableName;
2424
import com.facebook.presto.spi.TableHandle;
2525
import com.facebook.presto.spi.WarningCollector;
26+
import com.facebook.presto.spi.analyzer.ViewDefinitionReferences;
2627
import com.facebook.presto.spi.security.AccessControl;
2728
import com.facebook.presto.spi.security.ViewSecurity;
2829
import com.facebook.presto.sql.analyzer.Analysis;
@@ -92,9 +93,9 @@ public ListenableFuture<?> execute(CreateMaterializedView statement, Transaction
9293
accessControl.checkCanCreateView(session.getRequiredTransactionId(), session.getIdentity(), session.getAccessControlContext(), viewName);
9394

9495
Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
95-
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, warningCollector, query);
96+
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, warningCollector, query, new ViewDefinitionReferences());
9697
Analysis analysis = analyzer.analyzeSemantic(statement, false);
97-
checkAccessPermissions(analysis.getAccessControlReferences(), query, session.getPreparedStatements());
98+
checkAccessPermissions(analysis.getAccessControlReferences(), analysis.getViewDefinitionReferences(), query, session.getPreparedStatements(), session.getIdentity(), accessControl, session.getAccessControlContext());
9899

99100
List<ColumnMetadata> columnMetadata = analysis.getOutputDescriptor(statement.getQuery())
100101
.getVisibleFields().stream()

‎presto-main-base/src/main/java/com/facebook/presto/execution/CreateViewTask.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import com.facebook.presto.spi.MaterializedViewDefinition;
2323
import com.facebook.presto.spi.WarningCollector;
2424
import com.facebook.presto.spi.analyzer.ViewDefinition;
25+
import com.facebook.presto.spi.analyzer.ViewDefinitionReferences;
2526
import com.facebook.presto.spi.security.AccessControl;
2627
import com.facebook.presto.spi.security.AccessControlContext;
2728
import com.facebook.presto.spi.security.Identity;
@@ -125,9 +126,9 @@ public void queryPermissionCheck(AccessControl accessControl, Identity identity,
125126

126127
private Analysis analyzeStatement(Statement statement, Session session, Metadata metadata, AccessControl accessControl, List<Expression> parameters, WarningCollector warningCollector, String query)
127128
{
128-
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterExtractor(statement, parameters), warningCollector, query);
129+
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterExtractor(statement, parameters), warningCollector, query, new ViewDefinitionReferences());
129130
Analysis analysis = analyzer.analyzeSemantic(statement, false);
130-
checkAccessPermissions(analysis.getAccessControlReferences(), query, session.getPreparedStatements());
131+
checkAccessPermissions(analysis.getAccessControlReferences(), analysis.getViewDefinitionReferences(), query, session.getPreparedStatements(), session.getIdentity(), accessControl, session.getAccessControlContext());
131132

132133
return analysis;
133134
}

‎presto-main-base/src/main/java/com/facebook/presto/execution/DropFunctionTask.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import com.facebook.presto.metadata.Metadata;
1919
import com.facebook.presto.spi.MaterializedViewDefinition;
2020
import com.facebook.presto.spi.analyzer.ViewDefinition;
21+
import com.facebook.presto.spi.analyzer.ViewDefinitionReferences;
2122
import com.facebook.presto.spi.function.SqlFunctionId;
2223
import com.facebook.presto.spi.security.AccessControl;
2324
import com.facebook.presto.spi.security.AccessControlContext;
@@ -73,9 +74,9 @@ public String explain(DropFunction statement, List<Expression> parameters)
7374
public ListenableFuture<?> execute(DropFunction statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, QueryStateMachine stateMachine, List<Expression> parameters, String query)
7475
{
7576
Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
76-
Analyzer analyzer = new Analyzer(stateMachine.getSession(), metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, stateMachine.getWarningCollector(), query);
77+
Analyzer analyzer = new Analyzer(stateMachine.getSession(), metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, stateMachine.getWarningCollector(), query, new ViewDefinitionReferences());
7778
Analysis analysis = analyzer.analyzeSemantic(statement, false);
78-
checkAccessPermissions(analysis.getAccessControlReferences(), query, stateMachine.getSession().getPreparedStatements());
79+
checkAccessPermissions(analysis.getAccessControlReferences(), analysis.getViewDefinitionReferences(), query, stateMachine.getSession().getPreparedStatements(), stateMachine.getSession().getIdentity(), accessControl, stateMachine.getSession().getAccessControlContext());
7980

8081
Optional<List<TypeSignature>> parameterTypes = statement.getParameterTypes().map(types -> types.stream().map(TypeSignature::parseTypeSignature).collect(toImmutableList()));
8182

‎presto-main-base/src/main/java/com/facebook/presto/execution/SqlQueryExecution.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ private SqlQueryExecution(
223223
stateMachine.setExpandedQuery(queryAnalysis.getExpandedQuery());
224224

225225
stateMachine.beginColumnAccessPermissionChecking();
226-
checkAccessPermissions(queryAnalysis.getAccessControlReferences(), query, getSession().getPreparedStatements());
226+
checkAccessPermissions(queryAnalysis.getAccessControlReferences(), queryAnalysis.getViewDefinitionReferences(), query, getSession().getPreparedStatements(), getSession().getIdentity(), accessControl, getSession().getAccessControlContext());
227227
stateMachine.endColumnAccessPermissionChecking();
228228

229229
// when the query finishes cache the final query info, and clear the reference to the output stage

‎presto-main-base/src/main/java/com/facebook/presto/sql/analyzer/Analyzer.java‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import com.facebook.presto.common.QualifiedObjectName;
1818
import com.facebook.presto.metadata.Metadata;
1919
import com.facebook.presto.spi.WarningCollector;
20+
import com.facebook.presto.spi.analyzer.ViewDefinitionReferences;
2021
import com.facebook.presto.spi.function.FunctionHandle;
2122
import com.facebook.presto.spi.security.AccessControl;
2223
import com.facebook.presto.sql.parser.SqlParser;
@@ -59,6 +60,7 @@ public class Analyzer
5960
private final WarningCollector warningCollector;
6061
private final MetadataExtractor metadataExtractor;
6162
private final String query;
63+
private final ViewDefinitionReferences viewDefinitionReferences;
6264

6365
public Analyzer(
6466
Session session,
@@ -69,9 +71,10 @@ public Analyzer(
6971
List<Expression> parameters,
7072
Map<NodeRef<Parameter>, Expression> parameterLookup,
7173
WarningCollector warningCollector,
72-
String query)
74+
String query,
75+
ViewDefinitionReferences viewDefinitionReferences)
7376
{
74-
this(session, metadata, sqlParser, accessControl, queryExplainer, parameters, parameterLookup, warningCollector, Optional.empty(), query);
77+
this(session, metadata, sqlParser, accessControl, queryExplainer, parameters, parameterLookup, warningCollector, Optional.empty(), query, viewDefinitionReferences);
7578
}
7679

7780
public Analyzer(
@@ -84,7 +87,8 @@ public Analyzer(
8487
Map<NodeRef<Parameter>, Expression> parameterLookup,
8588
WarningCollector warningCollector,
8689
Optional<ExecutorService> metadataExtractorExecutor,
87-
String query)
90+
String query,
91+
ViewDefinitionReferences viewDefinitionReferences)
8892
{
8993
this.session = requireNonNull(session, "session is null");
9094
this.metadata = requireNonNull(metadata, "metadata is null");
@@ -97,6 +101,7 @@ public Analyzer(
97101
requireNonNull(metadataExtractorExecutor, "metadataExtractorExecutor is null");
98102
this.metadataExtractor = new MetadataExtractor(session, metadata, metadataExtractorExecutor, sqlParser, warningCollector);
99103
this.query = requireNonNull(query, "query is null");
104+
this.viewDefinitionReferences = requireNonNull(viewDefinitionReferences, "viewDefinitionReferences is null");
100105
}
101106

102107
public Analysis analyzeSemantic(Statement statement, boolean isDescribe)
@@ -109,8 +114,8 @@ public Analysis analyzeSemantic(
109114
Optional<QualifiedObjectName> procedureName,
110115
boolean isDescribe)
111116
{
112-
Statement rewrittenStatement = StatementRewrite.rewrite(session, metadata, sqlParser, queryExplainer, statement, parameters, parameterLookup, accessControl, warningCollector, query);
113-
Analysis analysis = new Analysis(rewrittenStatement, parameterLookup, isDescribe);
117+
Statement rewrittenStatement = StatementRewrite.rewrite(session, metadata, sqlParser, queryExplainer, statement, parameters, parameterLookup, accessControl, warningCollector, query, viewDefinitionReferences);
118+
Analysis analysis = new Analysis(rewrittenStatement, parameterLookup, isDescribe, viewDefinitionReferences);
114119

115120
metadataExtractor.populateMetadataHandle(session, rewrittenStatement, analysis.getMetadataHandle());
116121
analysis.setProcedureName(procedureName);
@@ -121,6 +126,11 @@ public Analysis analyzeSemantic(
121126
return analysis;
122127
}
123128

129+
public ViewDefinitionReferences getViewDefinitionReferences()
130+
{
131+
return viewDefinitionReferences;
132+
}
133+
124134
static void verifyNoAggregateWindowOrGroupingFunctions(
125135
Map<NodeRef<FunctionCall>, FunctionHandle> functionHandles,
126136
FunctionAndTypeResolver functionAndTypeResolver,

0 commit comments

Comments
 (0)