kasakrisz commented on code in PR #6700:
URL: https://github.com/apache/hive/pull/6700#discussion_r3802993966
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/command/CommandAuthorizerV1.java:
##########
@@ -232,14 +232,18 @@ private static void
getTablePartitionUsedColumns(HiveOperation op, BaseSemanticA
}
}
- private static void authorizeTable(HiveOperation op,
HiveAuthorizationProvider authorizer,
+ private static void authorizeTable(HiveOperation op, BaseSemanticAnalyzer
sem,
Review Comment:
How about passing only the `ColumnAccessInfo`? Does this method require
access to the entire `BaseSemanticAnalyzer` object?
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/TestSemanticAnalyzer.java:
##########
@@ -545,4 +546,60 @@ private void testMaterializeCTEUsesDDLFactory(boolean
cboEnabled) throws Excepti
cteAnalyzer[0] instanceof CreateTableAnalyzer);
}
}
+
+ @Test
+ public void testMaterializedCteInputsAndColumnAccess() throws Exception {
+ createKeyValueTable("src");
+
+ HiveConf testConf = new HiveConf(conf);
+ testConf.setIntVar(HiveConf.ConfVars.HIVE_CTE_MATERIALIZE_THRESHOLD, 1);
+
testConf.setBoolVar(HiveConf.ConfVars.HIVE_CTE_MATERIALIZE_FULL_AGGREGATE_ONLY,
false);
+ testConf.setBoolVar(HiveConf.ConfVars.HIVE_STATS_COLLECT_SCANCOLS, true);
+
+ SessionState.start(testConf);
Review Comment:
This test class has a setup block. Is it necessary for this test? If not,
please move this test to a separate test class and extract the initialization
into a `@Before / @BeforeEach` setup method.
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/TestSemanticAnalyzer.java:
##########
@@ -545,4 +546,60 @@ private void testMaterializeCTEUsesDDLFactory(boolean
cboEnabled) throws Excepti
cteAnalyzer[0] instanceof CreateTableAnalyzer);
}
}
+
+ @Test
+ public void testMaterializedCteInputsAndColumnAccess() throws Exception {
+ createKeyValueTable("src");
+
+ HiveConf testConf = new HiveConf(conf);
+ testConf.setIntVar(HiveConf.ConfVars.HIVE_CTE_MATERIALIZE_THRESHOLD, 1);
+
testConf.setBoolVar(HiveConf.ConfVars.HIVE_CTE_MATERIALIZE_FULL_AGGREGATE_ONLY,
false);
+ testConf.setBoolVar(HiveConf.ConfVars.HIVE_STATS_COLLECT_SCANCOLS, true);
+
+ SessionState.start(testConf);
+ String[] queries = {
+ "with q1 as ( select key from q2 where key = '5'),"
+ + "q2 as ( select key from src where key = '5') "
+ + "select * from (select key from q1) a",
+ "WITH q1 AS ("
+ + "WITH q2 AS (SELECT key, value FROM src WHERE key = '4') "
+ + "SELECT * FROM q2 UNION ALL SELECT * FROM q2) "
+ + "SELECT * FROM q1 t1 JOIN q1 t2 ON t1.key = t2.key"
+ };
+
+ SemanticAnalyzer[] analyzers = new SemanticAnalyzer[queries.length];
+ for (int i = 0; i < queries.length; i++) {
+ Context ctx = new Context(testConf);
+ ASTNode astNode = ParseUtils.parse(queries[i], ctx);
+ QueryState queryState = new
QueryState.Builder().withHiveConf(testConf).build();
+ SemanticAnalyzer analyzer = (SemanticAnalyzer)
SemanticAnalyzerFactory.get(queryState, astNode);
+ analyzer.initCtx(ctx);
+ analyzer.analyze(astNode, ctx);
+ analyzers[i] = analyzer;
+ }
+
+ for (int i = 0; i < analyzers.length; i++) {
+ SemanticAnalyzer analyzer = analyzers[i];
+ Set<ReadEntity> directInputs = analyzer.getInputs();
+ Set<ReadEntity> allInputs = analyzer.getAllInputs();
+
+ assertTrue("Materialized CTE should not expose base table in direct
inputs",
+ directInputs.stream().noneMatch(e -> isTableNamed(e, "src")));
+ assertTrue("Nested materialized CTE base table must appear in
getAllInputs",
+ allInputs.stream().anyMatch(e -> isTableNamed(e, "src")));
+
+ ColumnAccessInfo columnAccessInfo = analyzer.getColumnAccessInfo();
+ assertNotNull(columnAccessInfo);
+ List<String> srcCols =
columnAccessInfo.getTableToColumnAccessMap().get("default@src");
+ assertNotNull("Column must include nested materialized CTE base table",
srcCols);
+ assertTrue(srcCols.contains("key"));
+ if ( i == analyzers.length - 1) {
+ assertTrue(srcCols.contains("value"));
+ }
Review Comment:
This assertion seems to be required for only one of the queries. Please move
the queries into separate test methods and extract the common parts into
utility methods.
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/TestSemanticAnalyzer.java:
##########
@@ -545,4 +546,60 @@ private void testMaterializeCTEUsesDDLFactory(boolean
cboEnabled) throws Excepti
cteAnalyzer[0] instanceof CreateTableAnalyzer);
}
}
+
+ @Test
+ public void testMaterializedCteInputsAndColumnAccess() throws Exception {
+ createKeyValueTable("src");
+
+ HiveConf testConf = new HiveConf(conf);
+ testConf.setIntVar(HiveConf.ConfVars.HIVE_CTE_MATERIALIZE_THRESHOLD, 1);
+
testConf.setBoolVar(HiveConf.ConfVars.HIVE_CTE_MATERIALIZE_FULL_AGGREGATE_ONLY,
false);
+ testConf.setBoolVar(HiveConf.ConfVars.HIVE_STATS_COLLECT_SCANCOLS, true);
+
+ SessionState.start(testConf);
+ String[] queries = {
+ "with q1 as ( select key from q2 where key = '5'),"
+ + "q2 as ( select key from src where key = '5') "
+ + "select * from (select key from q1) a",
+ "WITH q1 AS ("
+ + "WITH q2 AS (SELECT key, value FROM src WHERE key = '4') "
+ + "SELECT * FROM q2 UNION ALL SELECT * FROM q2) "
+ + "SELECT * FROM q1 t1 JOIN q1 t2 ON t1.key = t2.key"
+ };
Review Comment:
What is the difference between these queries? Please write two separate test
methods with names that reflect the scope or goal of each test.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnAccessAnalyzer.java:
##########
@@ -34,7 +34,8 @@ public ColumnAccessAnalyzer(ParseContext pactx) {
pGraphContext = pactx;
}
- public ColumnAccessInfo analyzeColumnAccess(ColumnAccessInfo
columnAccessInfo) throws SemanticException {
+ public ColumnAccessInfo analyzeColumnAccess(SemanticAnalyzer analyzer)
throws SemanticException {
+ ColumnAccessInfo columnAccessInfo = analyzer.getColumnAccessInfo();
Review Comment:
Why was `ColumnAccessInfo` replaced with `SemanticAnalyzer`? The reference
`analyzer` is not used elsewhere in this method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
