Introduce ViewUnionAll to differentiate view-produced unions from subquery unions by quackaplop · Pull Request #143564 · elastic/elasticsearch

quackaplop · 2026-03-04T10:13:00Z

Summary

Creates ViewUnionAll extends UnionAll as a type marker for view-produced unions
ViewResolver now only skips ViewUnionAll, allowing plain UnionAll from subqueries to be processed normally for view resolution
The Fork handler uses replaceSubPlans() to preserve subclass types instead of constructing new Fork(...) directly, fixing a pre-existing type-erasure bug
createTopPlan() produces ViewUnionAll instead of UnionAll

Relates to esql-planning#181 (under "Loose ends and TODOs")

Test plan

Unit tests for ViewUnionAll copy methods (replaceChildren, replaceSubPlans, replaceSubPlansAndOutput preserve type)
Behavioral test verifying views inside user-written subqueries are resolved
CSV-spec tests combining views with subqueries (viewInSubquery, viewInSubqueryMultipleRows)
Existing view tests updated to assert ViewUnionAll
Existing subquery parser tests pass unchanged
Optimizer PushDownFilterAndLimitIntoUnionAllTests pass unchanged
CI integration tests (CsvIT, EsqlSpecIT)

Post-review changes

Re-do the ViewResolver changes after Views Security PR is merged
Change to >non-issue and remove changelog file
Add view name to ViewUnionAll (and add tests for name management)
Remove name from Subquery
Test subquery inside view definition

craigtaverner

If we can name the subqueries, we will complete the requirements. This could be done by enhancing UnionAll (discuss with @Fang), or adding names here. If we add names here we could also consider directly extending Fork, instead of UnionAll. Some more thinking might be required for that option.

Also, this PR needs to rebase on the security PR at #141050, or wait for that to merge and rebase on main. There will be conflicts.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/ViewUnionAll.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/view/ViewResolver.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/ViewUnionAll.java

...k/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/logical/ViewUnionAllTests.java

...ck/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/view/InMemoryViewServiceTests.java

craigtaverner · 2026-03-04T16:41:31Z

...ck/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/view/InMemoryViewServiceTests.java

+    // --- Behavioral test: views inside subqueries ---
+
+    public void testViewInsideSubqueryIsResolved() {
+        assumeTrue("Requires subquery in FROM command support", EsqlCapabilities.Cap.SUBQUERY_IN_FROM_COMMAND.isEnabled());


We could use VIEWS_WITH_BRANCHING here, since that depends on subqueries in from, and will also only turn on when both subqueries and views are out of snapshot.

...ck/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/view/InMemoryViewServiceTests.java

elasticsearchmachine · 2026-03-04T16:56:10Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

craigtaverner · 2026-03-09T14:45:44Z

docs/changelog/143564.yaml

@@ -0,0 +1,6 @@
+area: ES|QL
+issues:
+ - 143564


PR cannot refer to itself as an issue

craigtaverner · 2026-03-09T14:46:22Z

docs/changelog/143564.yaml

+ - 143564
+pr: 143564
+summary: Introduce ViewUnionAll to differentiate view-produced unions from subquery unions
+type: bug


For in-progress development we would rather mark this as non-issue to prevent an explosion of changelog entries for as-yet-unreleased work.

…query unions ViewResolver and the parser's subquery syntax both produce UnionAll nodes. Previously, ViewResolver skipped all UnionAll nodes assuming they were from view resolution, and the Fork handler lost the UnionAll type by rebuilding as plain Fork. This change: - Creates ViewUnionAll (extends UnionAll) as a type marker for view-produced unions - ViewResolver now only skips ViewUnionAll, allowing plain UnionAll from subqueries to be processed normally - The Fork handler uses replaceSubPlans() to preserve subclass types instead of constructing new Fork directly - createTopPlan() produces ViewUnionAll instead of UnionAll

Move ViewUnionAll unit tests from InMemoryViewServiceTests into a dedicated test file following the codebase convention of per-node test classes. Adds equals/hashCode and type-inequality tests.

The testAllCommandsWhitelistedOrBlacklisted test scans the classpath for all LogicalPlan subclasses and requires each to be listed. ViewUnionAll was missing, causing CI part-3 to fail.

fang-xing-esql

LGTM! I left a minor comment.

fang-xing-esql · 2026-03-10T22:25:39Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/view/ViewResolver.java

-                otherPlans.add(new Subquery(ur.source(), lp.plan, lp.name));
+                assert otherPlans.containsKey(lp.name) == false;
+                // TODO: Consider dropping Subquery nodes entirely
+                otherPlans.put(lp.name, new Subquery(ur.source(), lp.plan, lp.name));


It seems like the view name is saved in the hash map, shall we just use the subquery constructor without name here?

Yes, definitely. I had thought to make that change together with removing Subquery entirely (another PR), but could do it here too!

I've removed it here, and will remove Subquery as part of another PR.

craigtaverner · 2026-03-12T14:31:46Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/views.csv-spec

 12                 | false
 ;
+
+// Testing views used inside subqueries


Could also test subqueries inside views

Done inside the InMemoryViewServiceTests

Test views inside subqueries and subqueries inside views

craigtaverner

All previous comments have been taken care of now.

craigtaverner · 2026-03-16T09:41:26Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/views.csv-spec

 12                 | false
 ;
+
+// Testing views used inside subqueries


Done inside the InMemoryViewServiceTests

craigtaverner · 2026-03-16T09:42:46Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/view/ViewResolver.java

-            listener.delegateFailureAndWrap((l, rewritten) -> listener.onResponse(new ViewResolutionResult(rewritten, viewQueries)))
-        );
+        replaceViews(plan, parser, new LinkedHashSet<>(), viewQueries, 0, listener.delegateFailureAndWrap((l, rewritten) -> {
+            LogicalPlan postProcessed = rewriteUnionAllsWithNamedSubqueries(rewritten);


Perhaps add comments describing which use cases need this post-view-resolution update

Only because Subquery is there already

Taking into account Nik's new approach of using TestAnalyzer for maintaining test state.

quackaplop added a commit to quackaplop/elasticsearch that referenced this pull request Mar 4, 2026

Add changelog for elastic#143564

756b989

elasticsearchmachine added the v9.4.0 label Mar 4, 2026

quackaplop marked this pull request as ready for review March 4, 2026 16:11

quackaplop requested a review from craigtaverner March 4, 2026 16:11

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Mar 4, 2026

craigtaverner requested changes Mar 4, 2026

View reviewed changes

craigtaverner added >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL and removed needs:triage Requires assignment of a team area label labels Mar 4, 2026

craigtaverner reviewed Mar 9, 2026

View reviewed changes

quackaplop added 5 commits March 9, 2026 16:03

Add changelog for elastic#143564

9a597e6

Add dedicated ViewUnionAllTests for plan node unit tests

6091623

Move ViewUnionAll unit tests from InMemoryViewServiceTests into a dedicated test file following the codebase convention of per-node test classes. Adds equals/hashCode and type-inequality tests.

Add ViewUnionAll to ApproximationSupportTests unsupported commands list

8d38fa0

The testAllCommandsWhitelistedOrBlacklisted test scans the classpath for all LogicalPlan subclasses and requires each to be listed. ViewUnionAll was missing, causing CI part-3 to fail.

Change changelog type to bug

a2c3ce4

craigtaverner force-pushed the view_union_all_fork branch from 6db7690 to a2c3ce4 Compare March 9, 2026 15:04

craigtaverner added 3 commits March 9, 2026 17:57

Delete unneeded changelog file

e2d0505

Add names to subqueries in ViewUnionAll

d6ed962

Add more tests for named subqueries and fix bug with Map.hashCode

aa5dbfc

fang-xing-esql approved these changes Mar 10, 2026

View reviewed changes

craigtaverner reviewed Mar 12, 2026

View reviewed changes

fang-xing-esql mentioned this pull request Mar 13, 2026

ES|QL: Add (stack) telemetry for views #143745

Merged

Support view names inside ViewUnionAll

a27ccbc

Test views inside subqueries and subqueries inside views

craigtaverner approved these changes Mar 16, 2026

View reviewed changes

craigtaverner added 3 commits March 16, 2026 10:56

Fix EsqlNodeSubclassTests

9e9fb6d

Merge remote-tracking branch 'origin/main' into view_union_all_fork

8cc54f2

Ensure that ViewUnionAll survives through semantic analysis

5b6ae0a

craigtaverner added 2 commits March 16, 2026 12:46

Mark NamedSubquery unsupported in ApproximationSupportTests

46b23c3

Only because Subquery is there already

Merge branch 'main' into view_union_all_fork

ee52e42

craigtaverner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Mar 17, 2026

craigtaverner added 6 commits March 17, 2026 11:24

Merge branch 'main' into view_union_all_fork

145134d

Merge branch 'main' into view_union_all_fork

369eb46

Merge remote-tracking branch 'origin/main' into view_union_all_fork

efa88cb

Fixed AnalyzerTests after merging main

d04f4d2

Taking into account Nik's new approach of using TestAnalyzer for maintaining test state.

Merge branch 'main' into view_union_all_fork

c67a4c9

Merge branch 'main' into view_union_all_fork

3877822

craigtaverner merged commit dcf6860 into elastic:main Mar 23, 2026
35 of 36 checks passed

Conversation

quackaplop commented Mar 4, 2026 • edited by craigtaverner Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Post-review changes

Uh oh!

craigtaverner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fang-xing-esql left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craigtaverner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

quackaplop commented Mar 4, 2026 •

edited by craigtaverner

Loading