ADBDEV-4603: Allow multiple params in the child nodes of <dxl:TestExpr> #1140

mos65o2 · 2024-12-03T14:55:47Z

Allow multiple params in the child nodes of <dxl:TestExpr> (#1140)

Problem definition:
Some subqueries (example in the test) could output error "Attribute number
... not found in project list". This happened in the DXL to Plan Statement
stage because the attribute was not found in the list of column references
(CMappingColIdVar) when processing the left node of <dxl:TestExpr>.
<dxl:TestExpr> is part of the <dxl:SubPlan> node and the TestExpr acts
as a filter when a tuple is returned from SubPlan at execution
(see ExecScanSubPlan()).
<dxl:TestExpr> is processed by TranslateDXLSubplanTestExprToScalar(). The
left node was processed depending on the flag "has_outer_refs". If it was true,
the TranslateDXLTestExprScalarIdentToExpr() was called to read one <dxl:Ident>
and then convert it to a param. If it was false, the function of handling the
DXL tree was called without taking the param into account. In our case, the
"has_outer_refs" flag was false.
"has_outer_refs" was defined at the DXL generation stage in
PdxlnQuantifiedSubplan(). The params of TestExpr are the common columns of
the scalar and the inner part of Correlated Join node (see the physical plan).
In fact, the flag determined the presence of one param in the left node of
TestExpr. As it turned out, the left node can contain more than one param,
so the defenition of "has_outer_refs" was fixed.
However, another error occurred - "Unsupported subplan test expression"
since the left node contained a tree, but
TranslateDXLSubplanTestExprToScalar() only supported id and cast(id).
Therefore, a tree processing function was used to process the left node of
TestExpr. But in this case, it is impossible to get the TestExpr param
(it is read directly from node). Therefore, a list of params was used instead
of the "has_outer_refs" flag and params were pre-added to the list of column
references(CMappingColIdVar).

Problem summary:
The <dxl:TestExpr> handler could output errors:
"Attribute number ... not found in project list" and "Unsupported subplan
test expression".
The first error occurred if a reference to an attribute processed as a TestExpr
param (don't confuse with subplan params <dxl:ParamList>) was not found. This
occurred due to incorrect detection of the presence of TestExpr params
(m_outer_param flag) at the DXL generation stage in the
PdxlnQuantifiedSubplan function and the lack of adding TestExpr params to the
list of references to attributes (CMappingColIdVar) at the <dxl:TestExpr>
transforming stage.
The second error occurred if the left node dxl:TestExpr contained a tree with
the TestExpr param, since only id and cast(id) were supported (the param was
read directly from the node).
See the minidump for more information, but keep in mind that the TestExpr param
presence flag (m_outer_param) is not serialized.
Support for one param in the left node TestExpr was added in ac6ce1a.

Possible solutions:
The "Unsupported subplan test expression" error is fixed by using the
TranslateDXLToScalar function, which allows processing the tree in the child
nodes of TestExpr (mostly the left node). But in this case, it is impossible
to get the TestExpr param (currently it is read directly from node). It is
impossible to determine whether this is a param or a Var (there are no
markers indicating this), if there is more than one attribute in the
tree - this will cause the error "Attribute number ... not found in project
list". Therefore, the TestExpr params must be obtained in a separate structure
and added in advance to the list of references to attributes
(CMappingColIdVar). Defining the TestExpr params and adding them to the
CMappingColIdVar before handling DXL fixes the "Attribute number ... not
found in project list" error, but since the left node may contain a tree,
this will cause the "Unsupported subplan test expression" error. Therefore,
these errors must be fixed simultaneously.

Solution:

Define the necessary columns as TestExpr params at the DXL generating
stage;
Add references to attributes processed as TestExpr params at the DXL to
Plan Statment stage (prefilling CMappingColIdVar);
Add support for processing a tree with params in the left node TestExpr.

Changes:
At the DXL generation stage (in the PdxlnQuantifiedSubplan function),
the definition of the TestExpr params for PhysicalCorrelated***NLJoin
nodes has been unified. Instead of the "has_outer_refs" flag, the presence
of TestExpr params is now determined by the array of columns -
m_test_expr_params (passed via the CDXLScalarSubPlan class, not serialized).
At the DXL to Plan Statement stage in the subplan handler, a copy of the
mapping (list of references to columns) of the subplan is created to
handle <dxl:TestExpr>, including the TestExpr params (obtained from
m_test_expr_params). We use the copy because we do not want the TestExpr
params to be mixed with the subplan params.
In the <dxl:TestExpr> handler, the TranslateDXLToScalar function is
used instead of TranslateDXLTestExprScalarIdentToExpr for the left node,
which allows processing trees.
For the right node <dxl:TestExpr>, TranslateDXLToScalar is also used for
unification purposes. The TranslateDXLTestExprScalarIdentToExpr function
has been removed as unnecessary.

Problem: Subqueries using test expr may result in an "attribute not found in the projection list" error. Solution: Define the necessary parameters and support nested processing of the test expr tree. Changes: Adds parameter definition for test expression in exprToDXL step for CorrelatedInLeftSemiNLJoin. Reworked test expr handler in DXLtoPlStmt step.

Added a separate function to get projected columns from the Result node. PdxlRestictRestul returned to its original state.

BenderArenadata · 2024-12-27T22:48:51Z

Allure report https://allure.adsw.io/launch/90283

silent-observer · 2025-01-10T07:25:12Z

src/backend/gpopt/translate/CTranslatorDXLToScalar.cpp


 	const CDXLColRefArray *outer_refs = dxlop->GetDxlOuterColRefsArray();
-


Unrelated line removed

silent-observer · 2025-01-10T07:25:29Z

src/backend/gpopt/translate/CTranslatorDXLToScalar.cpp

@@ -2020,6 +1962,7 @@ CTranslatorDXLToScalar::TranslateDXLScalarIdentToScalar(
 				dxlop->GetDXLColRef()->Id()))
 	{
 		// not an outer ref -> Translate var node
+


Unrelated line added

silent-observer · 2025-01-10T07:25:45Z

src/backend/gporca/libgpopt/src/translate/CTranslatorExprToDXL.cpp

@@ -3089,7 +3089,7 @@ CTranslatorExprToDXL::PdxlnRestrictResult(CDXLNode *dxlnode,
 //
 //	@doc:
 //		Helper to build a Result expression with project list
-//		restricted to required columns
+//		restricted to required columns.


Unrelated comment changed

silent-observer · 2025-01-10T07:30:45Z

src/backend/gporca/libnaucrates/include/naucrates/dxl/operators/CDXLScalarSubPlan.h

@@ -61,20 +62,25 @@ class CDXLScalarSubPlan : public CDXLScalar
 	EdxlSubPlanType m_dxl_subplan_type;

 	// test expression -- not null if quantified/existential subplan
+


Unrelated line added

silent-observer · 2025-01-10T07:31:20Z

src/backend/gporca/libnaucrates/include/naucrates/dxl/operators/CDXLScalarSubPlan.h


 	// private copy ctor
 	CDXLScalarSubPlan(CDXLScalarSubPlan &);

 public:
 	// ctor/dtor
+


Unrelated line added

silent-observer · 2025-01-10T07:35:12Z

src/test/regress/sql/subselect.sql

@@ -614,3 +614,32 @@ drop table tl1;
 drop table tl2;
 drop table tl3;
 drop table tl4;
+
+-- Check deep tree support in Test expression node when outer params are present with =ANY or IN queries


Comment mentions =ANY queries, but only IN queries are actually tested. Maybe add tests for =ANY?

silent-observer · 2025-01-10T08:19:06Z

src/test/regress/sql/subselect.sql

+ANALYZE test_tbl_t;
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM test_tbl_t WHERE
+  (int_t IN (SELECT int_d FROM test_tbl_d)) IN (SELECT int_p = 1 from test_tbl_p) ORDER BY int_t;


Why do we need this second part of the test? It produces a slightly different plan without the patch, but the plan seems valid and no GPORCA errors are present.

QUERY PLAN ---------------------------------------------------------------------------------- Sort Output: test_tbl_t.text_t, test_tbl_t.int_t Sort Key: test_tbl_t.int_t -> Result Output: test_tbl_t.text_t, test_tbl_t.int_t Filter: (SubPlan 2) -> Gather Motion 3:1 (slice1; segments: 3) Output: test_tbl_t.text_t, test_tbl_t.int_t -> Seq Scan on public.test_tbl_t Output: test_tbl_t.text_t, test_tbl_t.int_t SubPlan 2 -> Result Output: true Filter: (((hashed SubPlan 1)) = ((test_tbl_p.int_p = 1))) -> Result Output: (test_tbl_p.int_p = 1), (hashed SubPlan 1) -> Materialize Output: test_tbl_p.int_p -> Gather Motion 3:1 (slice2; segments: 3) Output: test_tbl_p.int_p -> Seq Scan on public.test_tbl_p Output: test_tbl_p.int_p SubPlan 1 -> Result Output: test_tbl_d.int_d -> Materialize Output: test_tbl_d.int_d -> Gather Motion 3:1 (slice3; segments: 3) Output: test_tbl_d.int_d -> Seq Scan on public.test_tbl_d Output: test_tbl_d.int_d Optimizer: GPORCA (32 rows)

silent-observer · 2025-01-10T08:21:00Z

src/test/regress/sql/subselect.sql

+CREATE TABLE test_tbl_p (text_p text, int_p int) DISTRIBUTED BY (int_p);
+INSERT INTO test_tbl_p VALUES ('0', 0), ('0', 1);
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM test_tbl_t WHERE


It is not clear that falling back to Postgres optimizer is a fail condition, maybe add a comment that GPORCA should be able to handle this successfully? Or use set client_min_messages = log the way it's used in the ticket, so that the error is clearly visible in the diff

andr-sokolov · 2025-01-10T10:59:18Z

Typo: "physical plane"

Removed lines; Added "optimizer_trace_fallback" in test; Changed test output.

Previously, the projection of the internal node was redefined only for EopPhysicalCorrelatedLeftOuterNLJoin, but not for EopPhysicalCorrelatedInLeftSemiNLJoin and EopPhysicalCorrelatedNotInLeftAntiSemiNLJoin. This was due (most likely) to the fact that only for EopPhysicalCorrelatedLeftOuterNLJoin the <dxl:TestExpr> handler could correctly process the left node of <dxl:TestExpr> with one parameter. Since the TestExpr handler can now support trees in the left node with multiple parameters, the projection can be unified for the rest of the CorrelatedNLJoins. The logic for determining the projection of the columns of the internal node remains the same. Assert with the dxl_subplan_type definition remains the same.

BenderArenadata · 2025-01-31T15:23:54Z

Allure report https://allure.adsw.io/launch/92385

BenderArenadata · 2025-02-02T19:05:27Z

Allure report https://allure.adsw.io/launch/92429

BenderArenadata · 2025-02-03T11:51:01Z

Allure report https://allure.adsw.io/launch/92502

BenderArenadata · 2025-02-03T13:09:59Z

Allure report https://allure.adsw.io/launch/92514

mos65o2 changed the title ~~ADBDEV-4603: Fix attribute not found in project list subquery with ORCA~~ ADBDEV-4603: Fix ORCA attribute not found in project list subquery Dec 3, 2024

mos65o2 force-pushed the ADBDEV-4603 branch from 539791b to 08df301 Compare December 3, 2024 18:01

arenadata deleted a comment from BenderArenadata Dec 4, 2024

mos65o2 force-pushed the ADBDEV-4603 branch from 08df301 to dc10c68 Compare December 24, 2024 11:44

mos65o2 force-pushed the ADBDEV-4603 branch from dc10c68 to a57159d Compare December 24, 2024 14:55

Add testcase

5bed409

mos65o2 changed the title ~~ADBDEV-4603: Fix ORCA attribute not found in project list subquery~~ ADBDEV-4603: Fix ORCA attribute not found in project list Dec 25, 2024

arenadata deleted a comment from BenderArenadata Dec 25, 2024

mos65o2 marked this pull request as ready for review December 26, 2024 14:14

Simplify getting params from Result node

72cc8b2

Added a separate function to get projected columns from the Result node. PdxlRestictRestul returned to its original state.

mos65o2 marked this pull request as draft December 27, 2024 17:23

Add testcase

d0eeb6a

arenadata deleted a comment from BenderArenadata Dec 28, 2024

mos65o2 marked this pull request as ready for review December 28, 2024 10:32

mos65o2 requested review from silent-observer and andr-sokolov January 10, 2025 00:04

silent-observer reviewed Jan 10, 2025

View reviewed changes

mos65o2 changed the title ~~ADBDEV-4603: Fix ORCA attribute not found in project list~~ ADBDEV-4603: Allow multiple params in the child nodes <dxl:TestExpr> Jan 22, 2025

mos65o2 changed the title ~~ADBDEV-4603: Allow multiple params in the child nodes <dxl:TestExpr>~~ ADBDEV-4603: Allow multiple params in the child nodes of <dxl:TestExpr> Jan 30, 2025

mos65o2 added 3 commits January 31, 2025 11:59

Code refactoring

6744fd8

Removed lines; Added "optimizer_trace_fallback" in test; Changed test output.

Merge branch 'adb-6.x-dev' into ADBDEV-4603

6ca3f36

Change comments and correct typo

4c4fd6e

Merge branch 'adb-6.x-dev' into ADBDEV-4603

22ea8e3

Add GPOS_DEBUG for variables in ASSERT

d025d9b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADBDEV-4603: Allow multiple params in the child nodes of <dxl:TestExpr> #1140

ADBDEV-4603: Allow multiple params in the child nodes of <dxl:TestExpr> #1140

mos65o2 commented Dec 3, 2024 •

edited

Loading

BenderArenadata commented Dec 27, 2024

silent-observer Jan 10, 2025

silent-observer Jan 10, 2025

silent-observer Jan 10, 2025

silent-observer Jan 10, 2025

silent-observer Jan 10, 2025

silent-observer Jan 10, 2025

silent-observer Jan 10, 2025

silent-observer Jan 10, 2025

andr-sokolov commented Jan 10, 2025

BenderArenadata commented Jan 31, 2025

BenderArenadata commented Feb 2, 2025

BenderArenadata commented Feb 3, 2025

BenderArenadata commented Feb 3, 2025


		const CDXLColRefArray *outer_refs = dxlop->GetDxlOuterColRefsArray();

		@@ -61,20 +62,25 @@ class CDXLScalarSubPlan : public CDXLScalar
		EdxlSubPlanType m_dxl_subplan_type;

		// test expression -- not null if quantified/existential subplan

ADBDEV-4603: Allow multiple params in the child nodes of <dxl:TestExpr> #1140

Are you sure you want to change the base?

ADBDEV-4603: Allow multiple params in the child nodes of <dxl:TestExpr> #1140

Conversation

mos65o2 commented Dec 3, 2024 • edited Loading

BenderArenadata commented Dec 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andr-sokolov commented Jan 10, 2025

BenderArenadata commented Jan 31, 2025

BenderArenadata commented Feb 2, 2025

BenderArenadata commented Feb 3, 2025

BenderArenadata commented Feb 3, 2025

mos65o2 commented Dec 3, 2024 •

edited

Loading