This is an automated email from the ASF dual-hosted git repository. maxyang pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/cloudberry.git
commit a9e788857826041f2c01df47c758882c2d3a2628 Author: Chris Hajas <[email protected]> AuthorDate: Fri Mar 17 13:36:03 2023 -0700 Orca FIXME: Add checks to ensure plans with part selectors are valid (#15083) This commit adds back some checks to ensure we're not creating an invalid plan that will fail during execution. While I don't believe that we'll generate these plans currently as we don't currently generate partition selectors under NLJs, I'd rather be defensive here and maintain the old logic. I wasn't able to find any cases where these disallow incorrect plans based on the current logic. If we feel that these are redundant/unnecessary, I'm also ok with leaving these checks out for now. --- .../include/gpopt/base/CPartitionPropagationSpec.h | 5 ++++ .../src/base/CPartitionPropagationSpec.cpp | 28 ++++++++++++++++++++++ .../libgpopt/src/operators/CPhysicalMotion.cpp | 6 ++--- .../src/operators/CPhysicalPartitionSelector.cpp | 12 ++++------ .../libgpopt/src/operators/CPhysicalSpool.cpp | 7 ++---- 5 files changed, 41 insertions(+), 17 deletions(-) diff --git a/src/backend/gporca/libgpopt/include/gpopt/base/CPartitionPropagationSpec.h b/src/backend/gporca/libgpopt/include/gpopt/base/CPartitionPropagationSpec.h index 077473c8aa..92921bd389 100644 --- a/src/backend/gporca/libgpopt/include/gpopt/base/CPartitionPropagationSpec.h +++ b/src/backend/gporca/libgpopt/include/gpopt/base/CPartitionPropagationSpec.h @@ -183,6 +183,11 @@ public: // satisfies function BOOL FSatisfies(const CPartitionPropagationSpec *pps_reqd) const; + // Check if there is an unsupported part prop spec between two properties + BOOL IsUnsupportedPartSelector( + const CPartitionPropagationSpec *pps_reqd) const; + + SPartPropSpecInfo *FindPartPropSpecInfo(ULONG scan_id) const; diff --git a/src/backend/gporca/libgpopt/src/base/CPartitionPropagationSpec.cpp b/src/backend/gporca/libgpopt/src/base/CPartitionPropagationSpec.cpp index fb0d2deb91..2275d6cd92 100644 --- a/src/backend/gporca/libgpopt/src/base/CPartitionPropagationSpec.cpp +++ b/src/backend/gporca/libgpopt/src/base/CPartitionPropagationSpec.cpp @@ -342,6 +342,34 @@ CPartitionPropagationSpec::FSatisfies( return true; } +// Check if there is a matching partition propogation between two specs +// This is used to ensure that there aren't partition selectors in places that +// are unsupported by the executor +BOOL +CPartitionPropagationSpec::IsUnsupportedPartSelector( + const CPartitionPropagationSpec *pps_reqd) const +{ + if (pps_reqd->m_part_prop_spec_infos == nullptr) + { + return false; + } + + UlongToSPartPropSpecInfoMapIter hmulpi(pps_reqd->m_part_prop_spec_infos); + while (hmulpi.Advance()) + { + const SPartPropSpecInfo *reqd_info = hmulpi.Value(); + SPartPropSpecInfo *found_info = + FindPartPropSpecInfo(reqd_info->m_scan_id); + if (found_info != nullptr && + found_info->m_scan_id == reqd_info->m_scan_id && + found_info->m_type != reqd_info->m_type) + { + return true; + } + } + return false; +} + //--------------------------------------------------------------------------- // @function: // CPartitionPropagationSpec::AppendEnforcers diff --git a/src/backend/gporca/libgpopt/src/operators/CPhysicalMotion.cpp b/src/backend/gporca/libgpopt/src/operators/CPhysicalMotion.cpp index c89c73829d..602ae36115 100644 --- a/src/backend/gporca/libgpopt/src/operators/CPhysicalMotion.cpp +++ b/src/backend/gporca/libgpopt/src/operators/CPhysicalMotion.cpp @@ -42,13 +42,11 @@ CPhysicalMotion::FValidContext(CMemoryPool *, COptimizationContext *poc, GPOS_ASSERT(nullptr != pccBest); CDrvdPropPlan *pdpplanChild = pccBest->Pdpplan(); - // GPDB_12_MERGE_FIXME: Check partition propagation spec -#if 0 - if (pdpplanChild->Ppim()->FContainsUnresolved()) + CPartitionPropagationSpec *pps_req = poc->Prpp()->Pepp()->PppsRequired(); + if (pdpplanChild->Ppps()->IsUnsupportedPartSelector(pps_req)) { return false; } -#endif CEnfdDistribution *ped = poc->Prpp()->Ped(); if (ped->FCompatible(this->Pds()) && ped->FCompatible(pdpplanChild->Pds())) diff --git a/src/backend/gporca/libgpopt/src/operators/CPhysicalPartitionSelector.cpp b/src/backend/gporca/libgpopt/src/operators/CPhysicalPartitionSelector.cpp index 621f70be6e..93fa299b15 100644 --- a/src/backend/gporca/libgpopt/src/operators/CPhysicalPartitionSelector.cpp +++ b/src/backend/gporca/libgpopt/src/operators/CPhysicalPartitionSelector.cpp @@ -345,17 +345,13 @@ CPhysicalPartitionSelector::EpetDistribution(CExpressionHandle &exprhdl, return CEnfdProp::EpetUnnecessary; } - // GPDB_12_MERGE_FIXME: Check part propagation spec -#if 0 - CPartIndexMap *ppimDrvd = pdpplan->Ppim(); - if (!ppimDrvd->Contains(m_scan_id)) + CPartitionPropagationSpec *ppps = pdpplan->Ppps(); + + if (!ppps->Contains(m_scan_id)) { - // part consumer is defined above: prohibit adding a motion on top of the - // part resolver as this will create two slices + // ensure we don't create a plan with a motion on top of a partition selector return CEnfdProp::EpetProhibited; } -#endif - // part consumer found below: enforce distribution on top of part resolver return CEnfdProp::EpetRequired; } diff --git a/src/backend/gporca/libgpopt/src/operators/CPhysicalSpool.cpp b/src/backend/gporca/libgpopt/src/operators/CPhysicalSpool.cpp index fd79f9acf7..38675b336d 100644 --- a/src/backend/gporca/libgpopt/src/operators/CPhysicalSpool.cpp +++ b/src/backend/gporca/libgpopt/src/operators/CPhysicalSpool.cpp @@ -357,9 +357,6 @@ CPhysicalSpool::FValidContext(CMemoryPool *, COptimizationContext *poc, GPOS_ASSERT(nullptr != pccBest); CDrvdPropPlan *pdpplanChild = pccBest->Pdpplan(); - // GPDB_12_MERGE_FIXME: Check part propagation spec -#if 0 - // partition selections that happen outside of a physical spool does not do // any good on rescan: a physical spool blocks the rescan from the entire // subtree (in particular, any dynamic scan) underneath it. That means when @@ -387,11 +384,11 @@ CPhysicalSpool::FValidContext(CMemoryPool *, COptimizationContext *poc, // +--CScalarCmp (<) // |--CScalarIdent "a" (0) // +--CScalarIdent "partkey" (10) - if (pdpplanChild->Ppim()->FContainsUnresolved()) + CPartitionPropagationSpec *pps_req = poc->Prpp()->Pepp()->PppsRequired(); + if (pdpplanChild->Ppps()->IsUnsupportedPartSelector(pps_req)) { return false; } -#endif // Discard any context that is requesting for rewindability with motion hazard handling and // the physical spool is streaming with a motion underneath it. --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
