Add error handling to PexprScalarExactChild()

This function is only called in places where the exact child must exist, and returns null otherwise. However, there have been some cases where we assume the child must exist, but due to another bug the child does not and we don't handle the null. This is a defensive change; if we get into this situation we'll throw an exception and fall back to planner instead of crashing.
linbo-lin · Apr 2, 2021 · 45e36fe · 45e36fe
1 parent dd36090
commit 45e36fe
Show file tree

Hide file tree

Showing 11 changed files with 44 additions and 27 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -5,8  5,8 @@ project(gpopt LANGUAGES CXX C)
 
 set(CMAKE_CXX_STANDARD 98)
 set(GPORCA_VERSION_MAJOR 3)
-set(GPORCA_VERSION_MINOR 115)
-set(GPORCA_VERSION_PATCH 2)
 set(GPORCA_VERSION_MINOR 116)
 set(GPORCA_VERSION_PATCH 0)
 set(GPORCA_VERSION_STRING "${GPORCA_VERSION_MAJOR}.${GPORCA_VERSION_MINOR}.${GPORCA_VERSION_PATCH}")
 
 # Whenever an ABI-breaking change is made to GPORCA, this should be incremented.

diff --git a/libgpopt/include/gpopt/operators/CExpressionHandle.h b/libgpopt/include/gpopt/operators/CExpressionHandle.h
@@ -277,7  277,8 @@ class CExpressionHandle
 	CExpression *PexprScalarRep() const;
 
 	// return an exact scalar child at given index or return null if not possible
-	CExpression *PexprScalarExactChild(ULONG child_index) const;
 	CExpression *PexprScalarExactChild(ULONG child_index,
 									   BOOL error_on_null_return = false) const;
 
 	// return an exact scalar expression attached to handle or null if not possible
 	CExpression *PexprScalarExact() const;

diff --git a/libgpopt/src/base/CUtils.cpp b/libgpopt/src/base/CUtils.cpp
@@ -4745,9  4745,8 @@ CUtils::MakeJoinWithoutInferredPreds(CMemoryPool *mp, CExpression *join_expr)
 
 	CExpressionHandle expression_handle(mp);
 	expression_handle.Attach(join_expr);
-	CExpression *scalar_expr =
-		expression_handle.PexprScalarExactChild(join_expr->Arity() - 1);
-	GPOS_ASSERT(NULL != scalar_expr);
 	CExpression *scalar_expr = expression_handle.PexprScalarExactChild(
 		join_expr->Arity() - 1, true /*error_on_null_return*/);
 	CExpression *scalar_expr_without_inferred_pred =
 		CPredicateUtils::PexprRemoveImpliedConjuncts(mp, scalar_expr,
 													 expression_handle);

diff --git a/libgpopt/src/operators/CExpressionHandle.cpp b/libgpopt/src/operators/CExpressionHandle.cpp
@@ -1414,22  1414,36 @@ CExpressionHandle::PexprScalarRep() const
 // return an exact scalar child at given index or return null if not possible
 // (use this where exactness is required, e.g. for constraint derivation)
 CExpression *
-CExpressionHandle::PexprScalarExactChild(ULONG child_index) const
 CExpressionHandle::PexprScalarExactChild(ULONG child_index,
 										 BOOL error_on_null_return) const
 {
 	CExpression *result_expr = NULL;
 	if (NULL != m_pgexpr && !(*m_pgexpr)[child_index]->FScalarRepIsExact())
 	{
-		return NULL;
 		result_expr = NULL;
 	}
 
-	if (NULL != m_pexpr && NULL != (*m_pexpr)[child_index]->Pgexpr() &&
-		!((*m_pexpr)[child_index]->Pgexpr()->Pgroup()->FScalarRepIsExact()))
 	else if (NULL != m_pexpr && NULL != (*m_pexpr)[child_index]->Pgexpr() &&
 			 !((*m_pexpr)[child_index]
 				   ->Pgexpr()
 				   ->Pgroup()
 				   ->FScalarRepIsExact()))
 	{
 		// the expression does not come from a group, but its child does and
 		// the child group does not have an exact expression
-		return NULL;
 		result_expr = NULL;
 	}
 
-	return PexprScalarRepChild(child_index);
 	else
 	{
 		result_expr = PexprScalarRepChild(child_index);
 	}
 	if (NULL == result_expr && error_on_null_return)
 	{
 		GPOS_RAISE(CException::ExmaInvalid, CException::ExmiInvalid,
 				   GPOS_WSZ_LIT("Generated invalid plan with subquery"));
 	}
 	return result_expr;
 }
 
 // return an exact scalar expression attached to handle or null if not possible

diff --git a/libgpopt/src/operators/CPhysicalFilter.cpp b/libgpopt/src/operators/CPhysicalFilter.cpp
@@ -201,8  201,8 @@ CPhysicalFilter::PppsRequired(CMemoryPool *mp, CExpressionHandle &exprhdl,
 			ppimReqd, part_idx_id, CPartIndexMap::EppraPreservePropagators);
 
 		// look for a filter on the part key
-		CExpression *pexprScalar =
-			exprhdl.PexprScalarExactChild(1 /*child_index*/);
 		CExpression *pexprScalar = exprhdl.PexprScalarExactChild(
 			1 /*child_index*/, true /*error_on_null_return*/);
 
 		CExpression *pexprCmp = NULL;
 		CPartKeysArray *pdrgppartkeys = ppimReqd->Pdrgppartkeys(part_idx_id);
@@ -303,7  303,8 @@ CPhysicalFilter::PdsDerive(CMemoryPool *mp, CExpressionHandle &exprhdl) const
 	if (CDistributionSpec::EdtHashed == pdsChild->Edt() &&
 		exprhdl.HasOuterRefs())
 	{
-		CExpression *pexprFilterPred = exprhdl.PexprScalarExactChild(1);
 		CExpression *pexprFilterPred =
 			exprhdl.PexprScalarExactChild(1, true /*error_on_null_return*/);
 
 		CDistributionSpecHashed *pdshashedOriginal =
 			CDistributionSpecHashed::PdsConvert(pdsChild);

diff --git a/libgpopt/src/operators/CPhysicalFullMergeJoin.cpp b/libgpopt/src/operators/CPhysicalFullMergeJoin.cpp
@@ -71,7  71,8 @@ CPhysicalFullMergeJoin::PdsRequired(CMemoryPool *mp, CExpressionHandle &exprhdl,
 
 	BOOL nulls_collocated = true;
 	if (CPredicateUtils::ExprContainsOnlyStrictComparisons(
-			mp, exprhdl.PexprScalarExactChild(2)))
 			mp,
 			exprhdl.PexprScalarExactChild(2, true /*error_on_null_return*/)))
 	{
 		// There is no need to require NULL rows to be collocated if the merge clauses
 		// only contain STRICT operators. This is because any NULL row will automatically

diff --git a/libgpopt/src/operators/CPhysicalInnerNLJoin.cpp b/libgpopt/src/operators/CPhysicalInnerNLJoin.cpp
@@ -111,8  111,8 @@ CPhysicalInnerNLJoin::PdsRequired(CMemoryPool *mp, CExpressionHandle &exprhdl,
 			if (CDistributionSpec::EdtHashed == pdsOuter->Edt())
 			{
 				// require inner child to have matching hashed distribution
-				CExpression *pexprScPredicate =
-					exprhdl.PexprScalarExactChild(2);
 				CExpression *pexprScPredicate = exprhdl.PexprScalarExactChild(
 					2, true /*error_on_null_return*/);
 				CExpressionArray *pdrgpexpr =
 					CPredicateUtils::PdrgpexprConjuncts(mp, pexprScPredicate);
 

diff --git a/libgpopt/src/operators/CPhysicalJoin.cpp b/libgpopt/src/operators/CPhysicalJoin.cpp
@@ -1070,8  1070,8 @@ CPhysicalJoin::PppsRequiredCompute(CMemoryPool *mp, CExpressionHandle &exprhdl,
 			else
 			{
 				// check if there is an interesting condition involving the partition key
-				CExpression *pexprScalar =
-					exprhdl.PexprScalarExactChild(2 /*child_index*/);
 				CExpression *pexprScalar = exprhdl.PexprScalarExactChild(
 					2 /*child_index*/, true /*error_on_null_return*/);
 				AddFilterOnPartKey(mp, true /*fNLJoin*/, pexprScalar, ppim,
 								   ppfm, child_index, part_idx_id,
 								   fOuterPartConsumer, ppimResult, ppfmResult,
@@ -1097,8  1097,8 @@ CPhysicalJoin::PppsRequiredCompute(CMemoryPool *mp, CExpressionHandle &exprhdl,
 			else
 			{
 				// look for a filter on the part key
-				CExpression *pexprScalar =
-					exprhdl.PexprScalarExactChild(2 /*child_index*/);
 				CExpression *pexprScalar = exprhdl.PexprScalarExactChild(
 					2 /*child_index*/, true /*error_on_null_return*/);
 				AddFilterOnPartKey(mp, false /*fNLJoin*/, pexprScalar, ppim,
 								   ppfm, child_index, part_idx_id,
 								   fOuterPartConsumer, ppimResult, ppfmResult,

diff --git a/libgpopt/src/operators/CPhysicalLimit.cpp b/libgpopt/src/operators/CPhysicalLimit.cpp
@@ -173,8  173,8 @@ CPhysicalLimit::PdsRequired(CMemoryPool *mp, CExpressionHandle &exprhdl,
 			return PdsPassThru(mp, exprhdl, pdsInput, child_index);
 		}
 
-		CExpression *pexprOffset =
-			exprhdl.PexprScalarExactChild(1 /*child_index*/);
 		CExpression *pexprOffset = exprhdl.PexprScalarExactChild(
 			1 /*child_index*/, true /*error_on_null_return*/);
 		if (!m_fHasCount && CUtils::FScalarConstIntZero(pexprOffset))
 		{
 			// pass through input distribution if it has no count nor offset and is not

diff --git a/libgpopt/src/operators/CPhysicalScan.cpp b/libgpopt/src/operators/CPhysicalScan.cpp
@@ -180,8  180,8 @@ CPhysicalScan::PdsDerive(CMemoryPool *mp, CExpressionHandle &exprhdl) const
 		//
 		// This way the equiv spec stays incomplete only as long as it needs to be.
 
-		CExpression *pexprIndexPred =
-			exprhdl.PexprScalarExactChild(0 /*child_index*/);
 		CExpression *pexprIndexPred = exprhdl.PexprScalarExactChild(
 			0 /*child_index*/, true /*error_on_null_return*/);
 
 		CDistributionSpecHashed *pdshashed =
 			CDistributionSpecHashed::PdsConvert(m_pds);

diff --git a/libgpopt/src/xforms/CXformUtils.cpp b/libgpopt/src/xforms/CXformUtils.cpp
@@ -4191,7  4191,8 @@ CXformUtils::FJoinPredOnSingleChild(CMemoryPool *mp, CExpressionHandle &exprhdl)
 
 	GPOS_ASSERT(NULL != exprhdl.PexprScalarExactChild(arity - 1));
 	CExpressionArray *pdrgpexprPreds = CPredicateUtils::PdrgpexprConjuncts(
-		mp, exprhdl.PexprScalarExactChild(arity - 1));
 		mp, exprhdl.PexprScalarExactChild(arity - 1,
 										  true /*error_on_null_return*/));
 	const ULONG ulPreds = pdrgpexprPreds->Size();
 	BOOL fPredUsesSingleChild = false;
 	for (ULONG ulPred = 0; !fPredUsesSingleChild && ulPred < ulPreds; ulPred  )