Keep scalar and table macro references apart in the export order

An expression call can only name a scalar macro and a FROM-clause call
only a table macro, but the reference collector matched both entry kinds
by name, so a scalar and a table macro sharing one name formed a false
cycle whose fallback could emit a caller before its callee. References
now carry their call category: the callee of a TableFunctionRef is a
table reference, calls inside its arguments stay scalar, and candidates
are filtered by entry type.

Author: kryonix

src/execution/operator/persistent/physical_export.cpp

@@ -15,11 +15,13 @@
 #include "duckdb/function/scalar_macro_function.hpp"
 #include "duckdb/function/table_macro_function.hpp"
 #include "duckdb/common/set.hpp"
+#include "duckdb/common/unordered_set.hpp"
 #include "duckdb/parser/expression/function_expression.hpp"
 #include "duckdb/parser/expression/subquery_expression.hpp"
 #include "duckdb/parser/expression/window_expression.hpp"
 #include "duckdb/parser/parsed_expression_iterator.hpp"
 #include "duckdb/parser/statement/select_statement.hpp"
+#include "duckdb/parser/tableref/table_function_ref.hpp"
 
 #include <algorithm>
 #include <sstream>
@@ -180,43 +182,81 @@ struct MacroReference {
 	//! empty when the call is unqualified
 	string schema;
 	string name;
+	//! a FROM-clause table function call names a table macro, every other call a scalar one
+	bool is_table_function;
+	//! the expression the reference came from, to reclassify table function callees
+	const ParsedExpression *source;
 };
 
-static void CollectReferencedFunctionNames(ParsedExpression &expr, vector<MacroReference> &refs);
+struct MacroReferenceCollector {
+public:
+	vector<MacroReference> refs;
 
-static void CollectReferencedFunctionNames(QueryNode &node, vector<MacroReference> &refs) {
-	ParsedExpressionIterator::EnumerateQueryNodeChildren(
-	    node, [&refs](unique_ptr<ParsedExpression> &child) { CollectReferencedFunctionNames(*child, refs); });
-}
+public:
+	void Collect(ParsedExpression &expr) {
+		if (expr.GetExpressionClass() == ExpressionClass::FUNCTION) {
+			auto &function = expr.Cast<FunctionExpression>();
+			refs.push_back({function.GetQualifiedName().Schema().GetIdentifierName(),
+			                function.FunctionName().GetIdentifierName(), false, &expr});
+		} else if (expr.GetExpressionClass() == ExpressionClass::WINDOW) {
+			// a macro invoked with OVER is a window expression carrying the callee name itself
+			auto &window = expr.Cast<WindowExpression>();
+			refs.push_back({window.GetQualifiedName().Schema().GetIdentifierName(),
+			                window.FunctionName().GetIdentifierName(), false, &expr});
+		} else if (expr.GetExpressionClass() == ExpressionClass::SUBQUERY) {
+			Collect(*expr.Cast<SubqueryExpression>().Subquery()->node);
+		}
+		ParsedExpressionIterator::EnumerateChildren(expr, [this](ParsedExpression &child) { Collect(child); });
+	}
 
-static void CollectReferencedFunctionNames(ParsedExpression &expr, vector<MacroReference> &refs) {
-	if (expr.GetExpressionClass() == ExpressionClass::FUNCTION) {
-		auto &function = expr.Cast<FunctionExpression>();
-		refs.push_back(
-		    {function.GetQualifiedName().Schema().GetIdentifierName(), function.FunctionName().GetIdentifierName()});
-	} else if (expr.GetExpressionClass() == ExpressionClass::WINDOW) {
-		// a macro invoked with OVER is a window expression carrying the callee name itself
-		auto &window = expr.Cast<WindowExpression>();
-		refs.push_back(
-		    {window.GetQualifiedName().Schema().GetIdentifierName(), window.FunctionName().GetIdentifierName()});
-	} else if (expr.GetExpressionClass() == ExpressionClass::SUBQUERY) {
-		CollectReferencedFunctionNames(*expr.Cast<SubqueryExpression>().Subquery()->node, refs);
+	void Collect(QueryNode &node) {
+		ParsedExpressionIterator::EnumerateQueryNodeChildren(
+		    node, [this](unique_ptr<ParsedExpression> &child) { Collect(*child); },
+		    [this](TableRef &ref) {
+			    if (ref.type == TableReferenceType::TABLE_FUNCTION) {
+				    auto &table_function = ref.Cast<TableFunctionRef>();
+				    if (table_function.function) {
+					    table_function_roots.insert(table_function.function.get());
+				    }
+			    }
+		    });
 	}
-	ParsedExpressionIterator::EnumerateChildren(
-	    expr, [&refs](ParsedExpression &child) { CollectReferencedFunctionNames(child, refs); });
-}
+
+	//! reclassify the callees of table function refs: FROM f(...) names a table macro, while the
+	//! calls inside its arguments stay scalar. Must run while the walked tree is alive, the source
+	//! pointers dangle afterwards and are cleared here.
+	void Finalize() {
+		for (auto &ref : refs) {
+			if (table_function_roots.count(ref.source) != 0) {
+				ref.is_table_function = true;
+			}
+			ref.source = nullptr;
+		}
+	}
+
+private:
+	unordered_set<const ParsedExpression *> table_function_roots;
+};
 
 static vector<MacroReference> CollectMacroReferences(CatalogEntry &entry) {
 	vector<MacroReference> refs;
 	auto &macro_entry = entry.Cast<MacroCatalogEntry>();
 	for (auto &member : macro_entry.macros) {
-		// walk a copy: enumeration hands out mutable references
+		// one collector per member: Finalize compares pointers into the member's copy, so it must
+		// run before that copy is destroyed
+		MacroReferenceCollector collector;
 		if (member->type == MacroType::SCALAR_MACRO) {
+			// walk a copy: enumeration hands out mutable references
 			auto expression = member->Cast<ScalarMacroFunction>().expression->Copy();
-			CollectReferencedFunctionNames(*expression, refs);
+			collector.Collect(*expression);
+			collector.Finalize();
 		} else {
 			auto query_node = member->Cast<TableMacroFunction>().query_node->Copy();
-			CollectReferencedFunctionNames(*query_node, refs);
+			collector.Collect(*query_node);
+			collector.Finalize();
+		}
+		for (auto &ref : collector.refs) {
+			refs.push_back(std::move(ref));
 		}
 	}
 	return refs;
@@ -247,6 +287,11 @@ static void OrderMacrosByReference(catalog_entry_vector_t &macros) {
 				if (callee == i) {
 					continue;
 				}
+				// a FROM-clause call names a table macro and any other call a scalar one
+				const bool callee_is_table = macros[callee].get().type == CatalogType::TABLE_MACRO_ENTRY;
+				if (callee_is_table != ref.is_table_function) {
+					continue;
+				}
 				// a qualified call narrows the candidates to that schema
 				if (!ref.schema.empty() &&
 				    !StringUtil::CIEquals(ref.schema, macros[callee].get().ParentSchema().name.GetIdentifierName())) {

test/sql/export/export_macro_order.test

@@ -216,6 +216,50 @@ DROP MACRO TABLE tm_outer;
 statement ok
 DROP MACRO TABLE tm_inner;
 
+# a scalar and a table macro sharing one name stay apart: an expression call only references the
+# scalar entry, a FROM-clause call only the table entry
+
+statement ok
+CREATE MACRO f(x INTEGER) AS x;
+
+statement ok
+CREATE MACRO g(x INTEGER) AS main.f(x);
+
+statement ok
+CREATE MACRO f(x INTEGER) AS TABLE SELECT main.g(x) AS v;
+
+statement ok
+CREATE OR REPLACE MACRO g(x INTEGER) AS main.f(x) + 1;
+
+statement ok
+EXPORT DATABASE '{TEST_DIR}/export_macro_order_category' (FORMAT CSV);
+
+statement ok
+DROP MACRO TABLE f;
+
+statement ok
+DROP MACRO g;
+
+statement ok
+DROP MACRO f;
+
+statement ok
+IMPORT DATABASE '{TEST_DIR}/export_macro_order_category';
+
+query T
+SELECT v FROM f(1);
+----
+2
+
+statement ok
+DROP MACRO TABLE f;
+
+statement ok
+DROP MACRO g;
+
+statement ok
+DROP MACRO f;
+
 # a table macro used as MATCH_RECOGNIZE input is referenced through the recursed input ref
 
 statement ok