Mark array_sort with dynamic bool unshippable

In Postgres one can use a column value for the boolean value to
`array_sort()` that indicates whether the sort should be reversed. In
ClickHouse reverse sort requires a different function, so we can't use a
dynamic value. When 1f2dedd added `array_sort()`, it simply pretended
that a dynamic argument wasn't present and would always sort without
reverse, which could be surprising.

So move the determination of whether the boolean value is passed as a
dynamic expression from `deparseFuncExpr()`, which is called and
execution time, to `chfdw_is_shippable()`, which is called at planning
time. This requires a node to be passed to `chfdw_is_shippable()` so it
can evaluate the arguments, so add it as a parameter.

Move the setting of the function name to the `CF_ARRAY_SORT_DESC` case
of the switch statement in `deparseFuncExpr()` to ensure we don't
overwrite the name in the cache of function definitions kept by
`chfdw_check_for_custom_function()`.

Add a test to validate that a dynamic argument causes the expression not
to be passed down, and put it before the use of a constant to ensure
that the function name isn't overwritten.

Author: theory

src/deparse.c

@@ -421,7 +421,7 @@ foreign_expr_walker(Node * node,
 				 * can't be sent to remote because it might have incompatible
 				 * semantics on remote side.
 				 */
-				if (!chfdw_is_shippable(fe->funcid, ProcedureRelationId, fpinfo, &cdef))
+				if (!chfdw_is_shippable(node, fe->funcid, ProcedureRelationId, fpinfo, &cdef))
 					return false;
 
 				/*
@@ -472,7 +472,7 @@ foreign_expr_walker(Node * node,
 				 * (If the operator is shippable, we assume its underlying
 				 * function is too.)
 				 */
-				if (!chfdw_is_shippable(oe->opno, OperatorRelationId, fpinfo, NULL))
+				if (!chfdw_is_shippable(node, oe->opno, OperatorRelationId, fpinfo, NULL))
 					return false;
 
 				/*
@@ -589,7 +589,7 @@ foreign_expr_walker(Node * node,
 					return false;
 
 				/* As usual, it must be shippable. */
-				if (!chfdw_is_shippable(agg->aggfnoid, ProcedureRelationId, fpinfo, NULL))
+				if (!chfdw_is_shippable(node, agg->aggfnoid, ProcedureRelationId, fpinfo, NULL))
 					return false;
 
 				/* Features that ClickHouse doesn't support */
@@ -661,7 +661,7 @@ foreign_expr_walker(Node * node,
 					return false;
 
 				/* The window function must be shippable */
-				if (!chfdw_is_shippable(wfunc->winfnoid, ProcedureRelationId, fpinfo, NULL))
+				if (!chfdw_is_shippable(node, wfunc->winfnoid, ProcedureRelationId, fpinfo, NULL))
 					return false;
 
 				/* FILTER is not supported in ClickHouse window functions */
@@ -742,7 +742,7 @@ foreign_expr_walker(Node * node,
 	 * If result type of given expression is not shippable, it can't be sent
 	 * to remote because it might have incompatible semantics on remote side.
 	 */
-	if (check_type && !chfdw_is_shippable(exprType(node), TypeRelationId, fpinfo, NULL))
+	if (check_type && !chfdw_is_shippable(node, exprType(node), TypeRelationId, fpinfo, NULL))
 	{
 		return false;
 	}
@@ -2552,16 +2552,6 @@ deparseFuncExpr(FuncExpr * node, deparse_expr_cxt * context)
 	 * depends on boolean arg, resolve before printing.
 	 */
 	cdef = chfdw_check_for_custom_function(node->funcid);
-	if (cdef && cdef->cf_type == CF_ARRAY_SORT_DESC)
-	{
-		Expr	   *desc_arg = (Expr *) list_nth(node->args, 1);
-
-		if (IsA(desc_arg, Const) && !((Const *) desc_arg)->constisnull
-			&& DatumGetBool(((Const *) desc_arg)->constvalue))
-			strcpy(cdef->custom_name, "arrayReverseSort");
-		else
-			strcpy(cdef->custom_name, "arraySort");
-	}
 	cdef = appendFunctionName(node->funcid, context);
 
 	if (cdef)
@@ -2746,11 +2736,17 @@ deparseFuncExpr(FuncExpr * node, deparse_expr_cxt * context)
 				appendStringInfoChar(buf, ')');
 				return;
 			case CF_ARRAY_SORT_DESC:
-				/* name resolved before appendFunctionName, drop boolean arg */
-				appendStringInfoChar(buf, '(');
-				deparseExpr((Expr *) linitial(node->args), context);
-				appendStringInfoChar(buf, ')');
-				return;
+				{
+					/* Determine function name reverse boolean arg. */
+					Const	   *desc_arg = (Const *) list_nth(node->args, 1);
+
+					appendStringInfoString(buf, !desc_arg->constisnull && DatumGetBool(desc_arg->constvalue) ? "arrayReverseSort" : "arraySort");
+					appendStringInfoChar(buf, '(');
+					deparseExpr((Expr *) linitial(node->args), context);
+					appendStringInfoChar(buf, ')');
+					return;
+				}
+
 			case CF_ARRAY_FILL:
 				/* arrayWithConstant(n, val): swap args */
 				appendStringInfoChar(buf, '(');

src/include/fdw.h

@@ -371,7 +371,7 @@ extern Datum ch_timestamp_out(PG_FUNCTION_ARGS);
 extern Datum ch_date_out(PG_FUNCTION_ARGS);
 extern Datum ch_time_out(PG_FUNCTION_ARGS);
 
-extern bool chfdw_is_shippable(Oid objectId, Oid classId, CHFdwRelationInfo * fpinfo,
+extern bool chfdw_is_shippable(Node * node, Oid objectId, Oid classId, CHFdwRelationInfo * fpinfo,
 							   CustomObjectDef * *outcdef);
 extern double time_diff(struct timeval *prior, struct timeval *latter);
 

src/shipable.c

@@ -161,7 +161,7 @@ chfdw_is_builtin(Oid objectId)
  *	   Is this object (function/operator/type) shippable to foreign server?
  */
 bool
-chfdw_is_shippable(Oid objectId, Oid classId, CHFdwRelationInfo * fpinfo,
+chfdw_is_shippable(Node * node, Oid objectId, Oid classId, CHFdwRelationInfo * fpinfo,
 				   CustomObjectDef * *outcdef)
 {
 	ShippableCacheKey key;
@@ -192,6 +192,19 @@ chfdw_is_shippable(Oid objectId, Oid classId, CHFdwRelationInfo * fpinfo,
 		{
 			if (outcdef != NULL)
 				*outcdef = cdef;
+
+			if (cdef->cf_type == CF_ARRAY_SORT_DESC)
+			{
+				/*
+				 * If the boolean argument passed to array_sort(arr, bool) is
+				 * dynamic, we can't push it down.
+				 */
+				Expr	   *desc_arg = (Expr *) list_nth(((FuncExpr *) node)->args, 1);
+
+				if (!IsA(desc_arg, Const))
+					/* No support for a dynamic value here. */
+					return false;
+			}
 			return (cdef->cf_type != CF_UNSHIPPABLE);
 		}
 	}

test/expected/array_functions.out

@@ -201,6 +201,8 @@ NOTICE:  array_reverse PUSHED DOWN: t
 NOTICE:  (1,"{10,20,30}","{a,b,c}",aa-bb-cc)
 NOTICE:  array_sort PUSHED DOWN: f
 NOTICE:  (3,{60},{f},"Edit -> Insert -> Line Break")
+NOTICE:  array_sort(x, dynamic) NOT PUSHED DOWN: t
+NOTICE:  (1,"{10,20,30}","{a,b,c}",aa-bb-cc)
 NOTICE:  array_sort(x, true) PUSHED DOWN: t
 NOTICE:  (1,"{10,20,30}","{a,b,c}",aa-bb-cc)
 NOTICE:  array_sort(x, true, true) NOT PUSHED DOWN: t

test/sql/array_functions.sql

@@ -130,7 +130,14 @@ DECLARE
           "where": "array_sort(vals, true) = vals",
           "push": "((arraySort(vals) = vals))"
         }$_$,
-        -- PG18+ 'array_sort(vals, true) = ARRAY[30,20,10]'
+        -- PG18+ unshippable: 'array_sort(vals, dynamic) = ARRAY[30,20,10]'
+        -- Keep before array_sort(vals, const) to ensure function names not
+        -- replaced.
+        $_${
+          "func": "array_sort(x, dynamic)",
+          "where": "array_sort(vals, (select true)) = ARRAY[30,20,10]"
+        }$_$,
+        -- PG18+ 'array_sort(vals, const) = ARRAY[30,20,10]'
         $_${
           "func": "array_sort(x, true)",
           "where": "array_sort(vals, true) = ARRAY[30,20,10]",
@@ -166,6 +173,8 @@ BEGIN
         RAISE NOTICE '(1,"{10,20,30}","{a,b,c}",aa-bb-cc)';
         RAISE NOTICE 'array_sort PUSHED DOWN: f';
         RAISE NOTICE '(3,{60},{f},"Edit -> Insert -> Line Break")';
+        RAISE NOTICE 'array_sort(x, dynamic) NOT PUSHED DOWN: t';
+        RAISE NOTICE '(1,"{10,20,30}","{a,b,c}",aa-bb-cc)';
         RAISE NOTICE 'array_sort(x, true) PUSHED DOWN: t';
         RAISE NOTICE '(1,"{10,20,30}","{a,b,c}",aa-bb-cc)';
         RAISE NOTICE 'array_sort(x, true, true) NOT PUSHED DOWN: t';