HIVE-29559: materializeCTE to query Analyzer from DDLSemanticAnalyzerFactory

[konstantinb]

What changes were proposed in this pull request?

HIVE-29559: SemanticAnalyzer.materializeCTE to query SemanticAnalyzer instance from DDLSemanticAnalyzerFactory

Why are the changes needed?

As originally intended by the changes for HIVE-28724, the PR #5973 has introduced a new CreateTableAnalyzer class and registered it with DDLSemanticAnalyzerFactory

Unfortunately, the PR has also introduced a new CalcitePlanner.The materializeCTE() method is almost an exact copy of SemanticAnalyzer.materializeCTE(), with only one difference: it explicitly uses CreateTableAnalyzer instead of SemanticAnalyzer.

Because some code was removed from SemanticAnalyzer, its own implementation of materializeCTE() broke during the merge of this PR. The new query file of this PR reproduces the NPE happening with the current master branch code.

The first, "easy" and working choice was to copy the whole materializeCTE() from CalcitePlanner to SemanticAnalyzer and remove materializeCTE() from CalcitePlanner completely. I decided to take it a bit further due to the following reasons:

• The implementation of HIVE-28724:CREATE TABLE is not handled by DDLSemanticAnalyzerFactory #5973 used CreateTableAnalyzer directly, not via the factory, contradicting the original intent of HIVE-28724
• parent/child classes had a circular dependency (CreateTableAnalyzer extends CalcitePlanner, yet a method of CalcitePlanner explicitly creates an instance of CreateTableAnalyzer)

Does this PR introduce any user-facing change?

No

How was this patch tested?

Unit tests, new query file, all existing tests pass

[konstantinb]

I've left very verbose comments in here to better explain the scenario. I will gladly remove or shorten this comment section once reviewed

[kasakrisz]

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

[konstantinb]

TBH, I’m not sure it is necessary to add this .q file

@kasakrisz, while I can agree that testing the non-CBO configured path becomes redundant once full CBO deprecation gets addressed, I believe that having at least one new query that guards against similar regressions could be beneficial. The last query of the test file generates the following when executed against master: cte_materialize.q.test.log.txt

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.

The very first test in the file is not broken in the current master. I thought it would be useful to leave it in to confirm that my changes introduce no additional regressions for the CBO path, but you helped me to see that it is indeed redundant now

[konstantinb]

Hi @kasakrisz @abstractdog — follow-up to HIVE-28724. Tests passing, SonarQube clean. Would value your eyes when you have a chance.

Diff is mostly cleanup — net negative LOC. HIVE-28724 introduced DDLSemanticAnalyzerFactory routing for TOK_CREATETABLE and a CreateTableAnalyzer class that extends CalcitePlanner. Inside materializeCTE itself, however, the analyzer construction was hard-coded — new CreateTableAnalyzer(...) in CalcitePlanner.materializeCTE (the parent class instantiating its own subclass) and new SemanticAnalyzer(...) in SemanticAnalyzer.materializeCTE — leaving two near-duplicate methods and a parent↔child class reference. This PR routes that call through the factory in SemanticAnalyzer.materializeCTE, the two implementations become equivalent, the CalcitePlanner override is removed, and the cycle goes away. End state: factory-uniform with the original intent of HIVE-28724, just applied at this call site too.

The same asymmetry also produced an NPE on materialized CTEs whenever the runtime analyzer instance is a plain SemanticAnalyzer. The obvious trigger is set hive.cbo.enable=false, but it's also reachable from default-CBO-enabled entry via ReCompileWithoutCBOPlugin: with hive.cbo.fallback.strategy=ALWAYS, any non-fatal CBO crash on a query with a materialized CTE recompiles with cbo.enable=false injected into conf, landing in the buggy method. Both paths reproduced empirically; the regression test in cte_materialize.q covers each.

Aware of the non-CBO deprecation direction — this PR doesn't expand that surface, it removes a duplicate method. Happy to adjust the shape if you'd prefer something different.

cc @deniskuzZ @ramitg254 for visibility.

[deniskuzZ]

I’m not an expert in this area, but the change looks reasonable to me.

[kasakrisz]

I welcome this effort to remove duplicated code and have left comments on the test code.

[kasakrisz]

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

[kasakrisz]

How can this be verified from q test?

[konstantinb]

I agree that this is a bit too bold a statement - the test does confirm that the NPE is fixed, but does not really validate classes used

[kasakrisz]

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

[sonarqubecloud]

❌ The last analysis has failed.

See analysis details on SonarQube Cloud

[konstantinb]

@deniskuzZ, thank you for your feedback

@kasakrisz, thank you very much for your prompt review. I have modified the Java test code side as suggested. I have also reduced the number of tests in the .q file to a single test that runs into an NPE on the current branch, with CBO ON by default and ALWAYS as the fallback logic.

I believe this test provides value as long as the Non-CBO ReExec driver remains in the repository. The effort to remove all non-CBO execution paths is substantial and will likely take months, if not years. However, if you strongly feel that this .q file is unnecessary, I would be happy to delete it. Could you please let me know?