Copilot commented on code in PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#discussion_r3193847884
##########
src/backend/gporca/libgpopt/src/base/CUtils.cpp:
##########
@@ -978,6 +978,180 @@ CUtils::FHasCTEAnchor(CExpression *pexpr)
return false;
}
+// return CTEConsumers' and a set of CTEProducers' CTE ids in the given subtree
+void
+CUtils::CollectConsumersAndProducers(CMemoryPool *mp, CExpression *pexpr,
+
ULongPtrArray *cteConsumers,
+
UlongCteIdHashSet *cteProducerSet)
+{
Review Comment:
These new helpers are recursive but don’t call GPOS_CHECK_STACK_SIZE (unlike
nearby recursive walkers such as FHasCTEAnchor). Consider adding
GPOS_CHECK_STACK_SIZE at the top of CollectConsumersAndProducers() (and any
other recursive helpers added here) to avoid stack overflow on deep expression
trees.
##########
src/backend/gporca/libgpopt/src/base/CUtils.cpp:
##########
@@ -978,6 +978,180 @@ CUtils::FHasCTEAnchor(CExpression *pexpr)
return false;
}
+// return CTEConsumers' and a set of CTEProducers' CTE ids in the given subtree
+void
+CUtils::CollectConsumersAndProducers(CMemoryPool *mp, CExpression *pexpr,
+
ULongPtrArray *cteConsumers,
+
UlongCteIdHashSet *cteProducerSet)
+{
+ COperator *pop = pexpr->Pop();
+
+ if (COperator::EopPhysicalCTEConsumer == pop->Eopid())
+ {
+ cteConsumers->Append(GPOS_NEW(mp) ULONG(
+ CPhysicalCTEConsumer::PopConvert(pop)->UlCTEId()));
+ }
+ else if (COperator::EopPhysicalCTEProducer == pop->Eopid())
+ {
+ cteProducerSet->Insert(GPOS_NEW(mp) ULONG(
+ CPhysicalCTEProducer::PopConvert(pop)->UlCTEId()));
+ }
+
+ for (ULONG ul = 0; ul < pexpr->Arity(); ul++)
+ {
+ CExpression *pexprChild = (*pexpr)[ul];
+
+ if (!pexprChild->Pop()->FScalar())
+ {
+ CollectConsumersAndProducers(mp, pexprChild,
cteConsumers,
+
cteProducerSet);
+ }
+ }
+}
+
+BOOL
+CUtils::hasUnpairedCTEConsumer(CMemoryPool *mp, CExpression *pexpr)
+{
+ BOOL hasUnpairedConsumer = false;
+
+ ULongPtrArray *cteConsumers = GPOS_NEW(mp) ULongPtrArray(mp);
+ UlongCteIdHashSet *cteProducerSet = GPOS_NEW(mp) UlongCteIdHashSet(mp);
+
+ CollectConsumersAndProducers(mp, pexpr, cteConsumers, cteProducerSet);
+
+ // check if every consumer's producer is in ProducerSet
+ for (ULONG ul = 0; ul < cteConsumers->Size(); ul++)
+ {
+ if (!cteProducerSet->Contains((*cteConsumers)[ul]))
+ {
+ hasUnpairedConsumer = true;
+ break;
+ }
+ }
+ cteConsumers->Release();
+ cteProducerSet->Release();
+
+ return hasUnpairedConsumer;
+}
+
+// True if the distribution is replicated-like.
+static BOOL
+FReplicatedLikeDistribution(CDistributionSpec::EDistributionType edt)
+{
+ return (CDistributionSpec::EdtStrictReplicated == edt ||
+ CDistributionSpec::EdtTaintedReplicated == edt ||
+ CDistributionSpec::EdtUniversal == edt);
+}
+
+struct SCTEInfo
+{
+ ULONG cteId;
+ ULONG sliceId;
+
+ SCTEInfo(ULONG cte_id, ULONG slice_id) : cteId(cte_id),
sliceId(slice_id)
+ {
+ }
+};
+
+typedef CDynamicPtrArray<SCTEInfo, CleanupDelete<SCTEInfo> > CTEInfoArray;
+
+// Walk the physical tree, recording the slice id of every replicated
+// CTE Producer and every CTE Consumer. Slices are delimited by Motion
+// nodes: each non-scalar child of a Motion lives in a fresh slice --
+// same motId-stack idea as in apply_shareinput_xslice.
+static void
+CollectCTESlices(CMemoryPool *mp, CExpression *pexpr, ULONG curSlice,
+ ULONG *pNextSlice, CTEInfoArray *prodInfos,
+ CTEInfoArray *consInfos)
+{
Review Comment:
CollectCTESlices() is also recursive and currently lacks a
GPOS_CHECK_STACK_SIZE guard. Adding it at the top of the function would align
with other deep-tree walkers in this file and reduce the risk of stack overflow
on deeply nested plans.
##########
src/backend/gporca/libgpopt/include/gpopt/base/CUtils.h:
##########
@@ -1026,6 +1026,21 @@ class CUtils
static CTableDescriptorHashSet *RemoveDuplicateMdids(
CMemoryPool *mp, CTableDescriptorHashSet *tabdescs);
+ // hash set from CTE ids
+ typedef CHashSet<ULONG, gpos::HashValue<ULONG>, gpos::Equals<ULONG>,
+ CleanupDelete<ULONG> >
+ UlongCteIdHashSet;
+
+ static void CollectConsumersAndProducers(CMemoryPool *mp,
+
CExpression *pexpr,
+
ULongPtrArray *cteConsumers,
+
UlongCteIdHashSet *cteProducerSet);
+
+ static BOOL hasUnpairedCTEConsumer(CMemoryPool *mp, CExpression *pexpr);
Review Comment:
Naming is inconsistent with surrounding boolean helpers (e.g.,
FHasCTEAnchor, FHasSubqueryOrApply): hasUnpairedCTEConsumer() starts with a
lowercase 'h' and lacks the usual 'F' prefix. Consider renaming to match
existing conventions to keep the public CUtils API consistent.
##########
src/test/regress/sql/shared_scan.sql:
##########
@@ -120,3 +120,28 @@ where
(data_hour = date_trunc('day',data_hour) and stat.schema_name || '.'
||stat.table_name not in (select table_nm_23 from tbls_daily_report_23))
and (stat.schema_name || '.' ||stat.table_name not in (select
table_nm_onl_act from tbls_w_onl_actl_data))
or (stat.schema_name || '.' ||stat.table_name in (select
table_nm_onl_act from tbls_w_onl_actl_data));
+
+-- ORCA should fallback when a CTE over a replicated table is referenced
+-- from multiple scalar subqueries.
+-- ss_t1 needs enough rows (40000) to push ORCA to the cross-slice plan;
+-- with fewer rows the bug does not manifest and the test would silently
+-- pass even without the fix.
+CREATE TABLE ss_t1 AS
+ SELECT generate_series(1, 40000) id
+ DISTRIBUTED BY (id);
+CREATE TABLE ss_t2 AS
+ SELECT * FROM (VALUES (1, 10), (2, 20)) AS v(id, v)
+ DISTRIBUTED REPLICATED;
Review Comment:
The test creates ss_t1/ss_t2 without first dropping them. To make the script
more robust to partial runs or reruns (e.g., after a failure), consider adding
a guarded `DROP TABLE IF EXISTS ss_t1, ss_t2;` before the CREATE TABLE
statements (similar to earlier patterns in this file).
##########
src/backend/gporca/libgpopt/src/translate/CTranslatorExprToDXL.cpp:
##########
@@ -4250,6 +4264,10 @@ CTranslatorExprToDXL::BuildScalarSubplans(
{
const ULONG size = pdrgpcrInner->Size();
+ // Fallback to Postgres optimizer if the SubPlan's inner expression
contains a
+ // CTE Consumer whose Producer lives outside this subtree. Such a
Consumer
+ // would become a cross-slice Shared Scan reader without a local
Producer,
+ // which can hang the query or fail at execution time.
Review Comment:
This comment says the function will fall back to the Postgres optimizer when
the SubPlan’s inner expression contains an unpaired CTE Consumer, but there is
no corresponding check/raise in the code below. Either implement the described
fallback (e.g., by detecting the condition before translating) or adjust/remove
the comment to avoid misleading future maintainers.
##########
src/backend/gporca/libgpopt/include/gpopt/base/CUtils.h:
##########
@@ -1026,6 +1026,21 @@ class CUtils
static CTableDescriptorHashSet *RemoveDuplicateMdids(
CMemoryPool *mp, CTableDescriptorHashSet *tabdescs);
+ // hash set from CTE ids
+ typedef CHashSet<ULONG, gpos::HashValue<ULONG>, gpos::Equals<ULONG>,
+ CleanupDelete<ULONG> >
+ UlongCteIdHashSet;
+
+ static void CollectConsumersAndProducers(CMemoryPool *mp,
+
CExpression *pexpr,
+
ULongPtrArray *cteConsumers,
+
UlongCteIdHashSet *cteProducerSet);
+
+ static BOOL hasUnpairedCTEConsumer(CMemoryPool *mp, CExpression *pexpr);
+
+ static BOOL FHasCrossSliceReplicatedCTEConsumer(CMemoryPool *mp,
+
CExpression *pexpr);
Review Comment:
CollectConsumersAndProducers() / hasUnpairedCTEConsumer() are added to the
public CUtils API but appear unused in the codebase (no call sites found). If
they’re not needed, consider removing them or making them file-local to avoid
expanding the public surface area; if they are intended to be used, wiring them
into the relevant translation path would make that intent clear.
##########
src/backend/gporca/libgpopt/src/translate/CTranslatorExprToDXL.cpp:
##########
@@ -265,6 +265,20 @@ CTranslatorExprToDXL::PdxlnTranslate(CExpression *pexpr,
GPOS_ASSERT(nullptr == m_pdpplan);
+ // Walk the physical tree and detect a CTE Consumer placed on a
+ // different slice than its Producer when the Producer's output is
+ // replicated-like (StrictReplicated/TaintedReplicated/Universal).
+ // Fall back to the Postgres optimizer if it is detected because
+ // it breaks Producer-Consumer locality and can hang the
+ // query at execution.
+ if (CUtils::FHasCrossSliceReplicatedCTEConsumer(m_mp, pexpr))
+ {
+ GPOS_RAISE(
+ gpdxl::ExmaDXL, gpdxl::ExmiExpr2DXLUnsupportedFeature,
Review Comment:
This file generally qualifies DXL fallback exceptions via the gpopt
namespace (e.g., gpopt::ExmaDXL / gpopt::ExmiExpr2DXLUnsupportedFeature
elsewhere in this file). For consistency, consider using the same qualification
here as well (or unqualified, since gpdxl is brought into gpopt in the header).
--
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]
