Minor refactoring. Reworked fix for #3218 (Optimizer fails applying stream-local predicates before merging), now it covers both cases mentioned in the ticket.

src/jrd/optimizer/Optimizer.cpp

@@ -778,6 +778,9 @@ RecordSource* Optimizer::compile(BoolExprNodeStack* parentStack)
 				outerStreams.join(localStreams);
 			}
 
+			// Apply local booleans, if any
+			rsb = applyLocalBoolean(rsb, localStreams);
+
 			const auto river = FB_NEW_POOL(getPool()) River(csb, rsb, node, localStreams);
 			river->deactivate(csb);
 			rivers.add(river);
@@ -858,24 +861,24 @@ RecordSource* Optimizer::compile(BoolExprNodeStack* parentStack)
 			// no merge is made between a new cross river and the
 			// currently active rivers. Where in the new cross river
 			// a stream depends (index) on the active rivers.
-			StreamList dependent_streams, free_streams;
+			StreamList dependentStreams, freeStreams;
-			findDependentStreams(joinStreams, dependent_streams, free_streams);
+			findDependentStreams(joinStreams, dependentStreams, freeStreams);
 
 			// If we have dependent and free streams then we can't rely on
 			// the sort node to be used for index navigation
-			if (dependent_streams.hasData() && free_streams.hasData())
+			if (dependentStreams.hasData() && freeStreams.hasData())
 			{
 				sort = nullptr;
 				sortCanBeUsed = false;
 			}
 
-			if (dependent_streams.hasData())
+			if (dependentStreams.hasData())
 			{
 				// Copy free streams
-				joinStreams.assign(free_streams);
+				joinStreams.assign(freeStreams);
 
 				// Make rivers from the dependent streams
-				generateInnerJoin(dependent_streams, rivers, &sort, rse->rse_plan);
+				generateInnerJoin(dependentStreams, rivers, &sort, rse->rse_plan);
 
 				// Generate one river which holds a cross join rsb between
 				// all currently available rivers
@@ -885,7 +888,7 @@ RecordSource* Optimizer::compile(BoolExprNodeStack* parentStack)
 			}
 			else
 			{
-				if (free_streams.hasData())
+				if (freeStreams.hasData())
 				{
 					// Deactivate streams from rivers on stack, because
 					// the remaining streams don't have any indexed relationship with them
@@ -958,7 +961,7 @@ RecordSource* Optimizer::compile(BoolExprNodeStack* parentStack)
 		rsb = FB_NEW_POOL(getPool()) FirstRowsStream(csb, rsb, rse->rse_first);
 
 	if (rse->flags & RseNode::FLAG_SINGULAR)
-		rsb = FB_NEW_POOL(*tdbb->getDefaultPool()) SingularStream(csb, rsb);
+		rsb = FB_NEW_POOL(getPool()) SingularStream(csb, rsb);
 
 	if (rse->flags & RseNode::FLAG_WRITELOCK)
 	{
@@ -974,11 +977,11 @@ RecordSource* Optimizer::compile(BoolExprNodeStack* parentStack)
 				SCL_update, obj_relations, tail->csb_relation->rel_name);
 		}
 
-		rsb = FB_NEW_POOL(*tdbb->getDefaultPool()) LockedStream(csb, rsb);
+		rsb = FB_NEW_POOL(getPool()) LockedStream(csb, rsb);
 	}
 
 	if (rse->flags & RseNode::FLAG_SCROLLABLE)
-		rsb = FB_NEW_POOL(*tdbb->getDefaultPool()) BufferedStream(csb, rsb);
+		rsb = FB_NEW_POOL(getPool()) BufferedStream(csb, rsb);
 
 	return rsb;
 }
@@ -1455,12 +1458,13 @@ SortedStream* Optimizer::generateSort(const StreamList& streams,
 // Find conjuncts local to the given river and compose an appropriate filter
 //
 
-RecordSource* Optimizer::applyLocalBoolean(const River* river)
+RecordSource* Optimizer::applyLocalBoolean(RecordSource* rsb, const StreamList& streams)
 {
-	StreamStateHolder stateHolder(csb);
+	StreamStateHolder globalHolder(csb);
-	stateHolder.deactivate();
+	globalHolder.deactivate();
 
-	river->activate(csb);
+	StreamStateHolder localHolder(csb, streams);
+	localHolder.activate(csb);
 
 	BoolExprNode* boolean = nullptr;
 	double selectivity = MAXIMUM_SELECTIVITY;
@@ -1479,7 +1483,6 @@ RecordSource* Optimizer::applyLocalBoolean(const River* river)
 		}
 	}
 
-	const auto rsb = river->getRecordSource();
 	return boolean ? FB_NEW_POOL(getPool()) FilteredStream(csb, rsb, boolean, selectivity) : rsb;
 }
 
@@ -2203,7 +2206,7 @@ void Optimizer::formRivers(const StreamList& streams,
 // If the whole things is a moby no-op, return false.
 //
 
-bool Optimizer::generateEquiJoin(RiverList& org_rivers)
+bool Optimizer::generateEquiJoin(RiverList& orgRivers)
 {
 	ULONG selected_rivers[OPT_STREAM_BITS], selected_rivers2[OPT_STREAM_BITS];
 	ValueExprNode** eq_class;
@@ -2212,13 +2215,13 @@ bool Optimizer::generateEquiJoin(RiverList& org_rivers)
 	// a scratch block large enough to hold values to compute equality
 	// classes.
 
-	const unsigned cnt = (unsigned) org_rivers.getCount();
+	const unsigned orgCount = (unsigned) orgRivers.getCount();
 
-	if (cnt < 2)
+	if (orgCount < 2)
 		return false;
 
 	HalfStaticArray<ValueExprNode*, OPT_STATIC_ITEMS> scratch;
-	scratch.grow(baseConjuncts * cnt);
+	scratch.grow(baseConjuncts * orgCount);
 	ValueExprNode** classes = scratch.begin();
 
 	// Compute equivalence classes among streams. This involves finding groups
@@ -2231,47 +2234,15 @@ bool Optimizer::generateEquiJoin(RiverList& org_rivers)
 		if (iter & CONJUNCT_USED)
 			continue;
 
-		const auto cmpNode = nodeAs<ComparativeBoolNode>(*iter);
+		NestConst<ValueExprNode> node1;
+		NestConst<ValueExprNode> node2;
 
-		if (!cmpNode || (cmpNode->blrOp != blr_eql && cmpNode->blrOp != blr_equiv))
+		if (!getEquiJoinKeys(*iter, &node1, &node2, true))
 			continue;
 
-		ValueExprNode* node1 = cmpNode->arg1;
+		for (unsigned i = 0; i < orgRivers.getCount(); i++)
-		ValueExprNode* node2 = cmpNode->arg2;
-
-		dsc result, desc1, desc2;
-		node1->getDesc(tdbb, csb, &desc1);
-		node2->getDesc(tdbb, csb, &desc2);
-
-		// Ensure that arguments can be compared in the binary form
-		if (!CVT2_get_binary_comparable_desc(&result, &desc1, &desc2))
-			continue;
-
-		// Cast the arguments, if required
-		if (!DSC_EQUIV(&result, &desc1, true) || !DSC_EQUIV(&result, &desc2, true))
 		{
-			if (!DSC_EQUIV(&result, &desc1, true))
+			const auto river1 = orgRivers[i];
-			{
-				const auto cast = FB_NEW_POOL(getPool()) CastNode(getPool());
-				cast->source = node1;
-				cast->castDesc = result;
-				cast->impureOffset = csb->allocImpure<impure_value>();
-				node1 = cast;
-			}
-
-			if (!DSC_EQUIV(&result, &desc2, true))
-			{
-				const auto cast = FB_NEW_POOL(getPool()) CastNode(getPool());
-				cast->source = node2;
-				cast->castDesc = result;
-				cast->impureOffset = csb->allocImpure<impure_value>();
-				node2 = cast;
-			}
-		}
-
-		for (unsigned i = 0; i < org_rivers.getCount(); i++)
-		{
-			const auto river1 = org_rivers[i];
 
 			if (!river1->isReferenced(node1))
 			{
@@ -2283,13 +2254,13 @@ bool Optimizer::generateEquiJoin(RiverList& org_rivers)
 				node2 = temp;
 			}
 
-			for (unsigned j = i + 1; j < org_rivers.getCount(); j++)
+			for (unsigned j = i + 1; j < orgRivers.getCount(); j++)
 			{
-				const auto river2 = org_rivers[j];
+				const auto river2 = orgRivers[j];
 
 				if (river2->isReferenced(node2))
 				{
-					for (eq_class = classes; eq_class < last_class; eq_class += cnt)
+					for (eq_class = classes; eq_class < last_class; eq_class += orgCount)
 					{
 						if (fieldEqual(node1, classes[i]) ||
 							fieldEqual(node2, classes[j]))
@@ -2302,7 +2273,7 @@ bool Optimizer::generateEquiJoin(RiverList& org_rivers)
 					eq_class[j] = node2;
 
 					if (eq_class == last_class)
-						last_class += cnt;
+						last_class += orgCount;
 				}
 			}
 		}
@@ -2312,23 +2283,23 @@ bool Optimizer::generateEquiJoin(RiverList& org_rivers)
 	// Obviously, if the set of classes is empty, return false
 	// to indicate that nothing could be done.
 
-	unsigned river_cnt = 0;
+	unsigned riverCount = 0;
-	HalfStaticArray<ValueExprNode**, OPT_STATIC_ITEMS> selected_classes(cnt);
+	HalfStaticArray<ValueExprNode**, OPT_STATIC_ITEMS> selected_classes(orgCount);
 
-	for (eq_class = classes; eq_class < last_class; eq_class += cnt)
+	for (eq_class = classes; eq_class < last_class; eq_class += orgCount)
 	{
-		unsigned i = getRiverCount(cnt, eq_class);
+		unsigned i = getRiverCount(orgCount, eq_class);
 
-		if (i > river_cnt)
+		if (i > riverCount)
 		{
-			river_cnt = i;
+			riverCount = i;
 			selected_classes.shrink(0);
 			selected_classes.add(eq_class);
-			classMask(cnt, eq_class, selected_rivers);
+			classMask(orgCount, eq_class, selected_rivers);
 		}
 		else
 		{
-			classMask(cnt, eq_class, selected_rivers2);
+			classMask(orgCount, eq_class, selected_rivers2);
 
 			for (i = 0; i < OPT_STREAM_BITS; i++)
 			{
@@ -2341,30 +2312,21 @@ bool Optimizer::generateEquiJoin(RiverList& org_rivers)
 		}
 	}
 
-	if (!river_cnt)
+	if (!riverCount)
 		return false;
 
-	// AB: Deactivate currently all streams from every river, because we
-	// need to know which nodes are computable between the rivers used
-	// for the merge.
-
-	StreamStateHolder stateHolder(csb);
-	stateHolder.deactivate();
-
 	HalfStaticArray<RecordSource*, OPT_STATIC_ITEMS> rsbs;
 	HalfStaticArray<NestValueArray*, OPT_STATIC_ITEMS> keys;
 
-	// Unconditionally disable merge joins in favor of hash joins.
+	bool useMergeJoin = false;
-	// This is a temporary debugging measure.
-	bool prefer_merge_over_hash = false;
 
 	// AB: Get the lowest river position from the rivers that are merged
 
-	RiverList rivers_to_merge;
+	StreamList streams;
-	unsigned lowest_river_position = MAX_ULONG;
+	RiverList rivers;
-	unsigned number = 0;
+	unsigned number = 0, lowestPosition = MAX_ULONG;
 
-	for (River** iter = org_rivers.begin(); iter < org_rivers.end(); number++)
+	for (River** iter = orgRivers.begin(); iter < orgRivers.end(); number++)
 	{
 		River* const river = *iter;
 
@@ -2374,21 +2336,20 @@ bool Optimizer::generateEquiJoin(RiverList& org_rivers)
 			continue;
 		}
 
-		if (number < lowest_river_position)
+		if (number < lowestPosition)
-			lowest_river_position = number;
+			lowestPosition = number;
 
-		rivers_to_merge.add(river);
+		streams.join(river->getStreams());
-		org_rivers.remove(iter);
+		rivers.add(river);
+		orgRivers.remove(iter);
 
-		// Apply local river booleans, if any
+		auto rsb = river->getRecordSource();
-
-		auto rsb = applyLocalBoolean(river);
 
 		// Collect RSBs and keys to join
 
 		const auto key = FB_NEW_POOL(getPool()) SortNode(getPool());
 
-		if (prefer_merge_over_hash)
+		if (useMergeJoin)
 		{
 			ValueExprNode*** selected_class;
 
@@ -2400,9 +2361,7 @@ bool Optimizer::generateEquiJoin(RiverList& org_rivers)
 				key->expressions.add((*selected_class)[number]);
 			}
 
-			StreamList streams;
+			rsb = generateSort(river->getStreams(), nullptr, rsb, key, favorFirstRows(), false);
-			streams.assign(river->getStreams());
-			rsb = generateSort(streams, nullptr, rsb, key, favorFirstRows(), false);
 		}
 		else
 		{
@@ -2419,7 +2378,7 @@ bool Optimizer::generateEquiJoin(RiverList& org_rivers)
 		// For a hash join, we need to choose the smallest ones as inner sub-streams,
 		// hence we reverse the order when storing them in the temporary arrays.
 
-		if (prefer_merge_over_hash)
+		if (useMergeJoin)
 		{
 			rsbs.add(rsb);
 			keys.add(&key->expressions);
@@ -2437,7 +2396,7 @@ bool Optimizer::generateEquiJoin(RiverList& org_rivers)
 
 	RecordSource* rsb = nullptr;
 
-	if (prefer_merge_over_hash)
+	if (useMergeJoin)
 	{
 		rsb = FB_NEW_POOL(getPool())
 			MergeJoin(csb, rsbs.getCount(), (SortedStream**) rsbs.begin(), keys.begin());
@@ -2448,36 +2407,11 @@ bool Optimizer::generateEquiJoin(RiverList& org_rivers)
 			HashJoin(tdbb, csb, rsbs.getCount(), rsbs.begin(), keys.begin());
 	}
 
-	// Activate streams of all the rivers being merged
-
-	for (const auto river : rivers_to_merge)
-		river->activate(csb);
-
 	// Pick up any boolean that may apply
+	rsb = applyLocalBoolean(rsb, streams);
 
-	BoolExprNode* boolean = nullptr;
+	const auto finalRiver = FB_NEW_POOL(getPool()) River(csb, rsb, rivers);
-	double selectivity = MAXIMUM_SELECTIVITY;
+	orgRivers.insert(lowestPosition, finalRiver);
-
-	for (auto iter = getBaseConjuncts(); iter.hasData(); ++iter)
-	{
-		if (!(iter & CONJUNCT_USED) &&
-			!(iter->nodFlags & ExprNode::FLAG_RESIDUAL) &&
-			iter->computable(csb, INVALID_STREAM, false))
-		{
-			compose(getPool(), &boolean, iter);
-			iter |= CONJUNCT_USED;
-
-			if (!(iter & CONJUNCT_MATCHED))
-				selectivity *= getSelectivity(*iter);
-		}
-	}
-
-	if (boolean)
-		rsb = FB_NEW_POOL(getPool()) FilteredStream(csb, rsb, boolean, selectivity);
-
-	const auto merged_river = FB_NEW_POOL(getPool()) River(csb, rsb, rivers_to_merge);
-
-	org_rivers.insert(lowest_river_position, merged_river);
 
 	return true;
 }
@@ -2882,6 +2816,68 @@ RecordSource* Optimizer::generateRetrieval(StreamType stream,
 }
 
 
+//
+// Check whether the given boolean can be involved in a equi-join relationship
+//
+
+bool Optimizer::getEquiJoinKeys(BoolExprNode* boolean,
+								NestConst<ValueExprNode>* node1,
+								NestConst<ValueExprNode>* node2,
+								bool needCast)
+{
+	auto cmpNode = nodeAs<ComparativeBoolNode>(boolean);
+	if (!cmpNode || (cmpNode->blrOp != blr_eql && cmpNode->blrOp != blr_equiv))
+		return false;
+
+	auto arg1 = cmpNode->arg1;
+	auto arg2 = cmpNode->arg2;
+
+	if (!getEquiJoinKeys(arg1, arg2, needCast))
+		return false;
+
+	*node1 = arg1;
+	*node2 = arg2;
+	return true;
+}
+
+bool Optimizer::getEquiJoinKeys(NestConst<ValueExprNode>& node1,
+								NestConst<ValueExprNode>& node2,
+								bool needCast)
+{
+	dsc result, desc1, desc2;
+	node1->getDesc(tdbb, csb, &desc1);
+	node2->getDesc(tdbb, csb, &desc2);
+
+	// Ensure that arguments can be compared in the binary form
+	if (!CVT2_get_binary_comparable_desc(&result, &desc1, &desc2))
+		return false;
+
+	// Cast the arguments to the common data type, if required
+	if (needCast)
+	{
+		if (!DSC_EQUIV(&result, &desc1, true))
+		{
+			const auto cast = FB_NEW_POOL(getPool()) CastNode(getPool());
+			cast->source = node1;
+			cast->castDesc = result;
+			cast->impureOffset = csb->allocImpure<impure_value>();
+			node1 = cast;
+		}
+
+		if (!DSC_EQUIV(&result, &desc2, true))
+		{
+			const auto cast = FB_NEW_POOL(getPool()) CastNode(getPool());
+			cast->source = node2;
+			cast->castDesc = result;
+			cast->impureOffset = csb->allocImpure<impure_value>();
+			node2 = cast;
+		}
+	}
+
+	return true;
+}
+
+
 //
 // Make an alias string suitable for printing as part of the plan.
 // For views, this means multiple aliases to distinguish the base table.

src/jrd/optimizer/Optimizer.h

@@ -156,12 +156,10 @@ class River
 {
 public:
 	River(CompilerScratch* csb, RecordSource* rsb, RecordSourceNode* node, const StreamList& streams)
-		: m_rsb(rsb), m_nodes(csb->csb_pool), m_streams(csb->csb_pool)
+		: m_rsb(rsb), m_nodes(csb->csb_pool), m_streams(csb->csb_pool, streams)
 	{
 		if (node)
 			m_nodes.add(node);
-
-		m_streams.assign(streams);
 	}
 
 	River(CompilerScratch* csb, RecordSource* rsb, RiverList& rivers)
@@ -410,6 +408,14 @@ public:
 		return (rse->flags & RseNode::FLAG_OPT_FIRST_ROWS) != 0;
 	}
 
+	bool getEquiJoinKeys(BoolExprNode* boolean,
+						 NestConst<ValueExprNode>* node1,
+						 NestConst<ValueExprNode>* node2,
+						 bool needCast = false);
+	bool getEquiJoinKeys(NestConst<ValueExprNode>& node1,
+						 NestConst<ValueExprNode>& node2,
+						 bool needCast = false);
+
 	Firebird::string makeAlias(StreamType stream);
 	void printf(const char* format, ...);
 
@@ -418,7 +424,7 @@ private:
 
 	RecordSource* compile(BoolExprNodeStack* parentStack);
 
-	RecordSource* applyLocalBoolean(const River* river);
+	RecordSource* applyLocalBoolean(RecordSource* rsb, const StreamList& streams);
 	void checkIndices();
 	void checkSorts();
 	unsigned decompose(BoolExprNode* boolNode, BoolExprNodeStack& stack);