kasakrisz commented on code in PR #6423:
URL: https://github.com/apache/hive/pull/6423#discussion_r3166891889
##########
ql/src/test/org/apache/hadoop/hive/ql/parse/TestSemanticAnalyzer.java:
##########
@@ -493,4 +499,51 @@ private void checkTablesUsed(String query, Set<String>
tables) throws Exception
Assert.assertEquals(new TreeSet<>(tables), new TreeSet<>(result));
}
+
+ @Test
+ public void testMaterializeCTEWithCBODisabled() throws Exception {
+ testMaterializeCTEUsesDDLFactory(false);
+ }
+
+ @Test
+ public void testMaterializeCTEWithCBOEnabled() throws Exception {
+ testMaterializeCTEUsesDDLFactory(true);
+ }
+
+ private void testMaterializeCTEUsesDDLFactory(boolean cboEnabled) throws
Exception {
+ HiveConf testConf = new HiveConf(conf);
+ testConf.setBoolVar(HiveConf.ConfVars.HIVE_CBO_ENABLED, cboEnabled);
+
+ SessionState.start(testConf);
+ Context ctx = new Context(testConf);
+
+ // Reference CTE 3 times to exceed default materialization threshold of 2
+ String query = "WITH cte AS (SELECT COUNT(*) as cnt FROM table1) " +
+ "SELECT * FROM cte UNION ALL SELECT * FROM cte UNION ALL
SELECT * FROM cte";
Review Comment:
You can also lower the threshold (`hive.optimize.cte.materialize.threshold`)
to `1` in the scope of this test case and use a simpler query
##########
ql/src/test/queries/clientpositive/cte_materialize.q:
##########
@@ -0,0 +1,46 @@
+-- Test CTE materialization with both CBO enabled and disabled
+-- Verifies DDLSemanticAnalyzerFactory is used for CTE materialization
+-- Also ensures that an NPE is no longer triggered with CBO off (HIVE-28724
regression)
+
+-- Test with CBO enabled (default)
+explain
+WITH cte AS (
+ SELECT COUNT(*) as cnt FROM (SELECT 1 as id) t
+)
+SELECT * FROM cte
+UNION ALL
+SELECT * FROM cte
+UNION ALL
+SELECT * FROM cte;
+
+-- Test with CBO disabled
+set hive.cbo.enable=false;
+
+explain
+WITH cte AS (
+ SELECT COUNT(*) as cnt FROM (SELECT 1 as id) t
+)
+SELECT * FROM cte
+UNION ALL
+SELECT * FROM cte
+UNION ALL
+SELECT * FROM cte;
+
+-- Test the recompile-without-CBO auto-trigger path.
Review Comment:
Based on the scope of this patch, this path does not need to be covered
because the previous test case already covers the non-CBO path.
TBH, I’m not sure it is necessary to add this `.q` file, since we already
have many tests for CTE materialization (`cte_mat_*.q`). Those tests cover the
CBO path, which should always be the default. In general, the non-CBO path is
no longer supported.
https://issues.apache.org/jira/browse/HIVE-27830
https://issues.apache.org/jira/browse/HIVE-28741
##########
ql/src/test/queries/clientpositive/cte_materialize.q:
##########
@@ -0,0 +1,46 @@
+-- Test CTE materialization with both CBO enabled and disabled
+-- Verifies DDLSemanticAnalyzerFactory is used for CTE materialization
Review Comment:
How can this be verified from q test?
--
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]
