Retain SQL invocation information through binding and optimization - #25720

Why is this necessary when we have the AggregateFunctionSet already?

I use this to distinguish the definitions installed when the catalog entry was created from later replacements. AggregateFunctionSet::ApplyToFunctions() replaces each definition with a modified copy, so the live set no longer retains that identity. The snapshot shares the original definitions rather than copying the function objects. This, too, is relevant in the follow up when validating PIVOT's internal list aggregate, where recognizing the name and signature alone is insufficient.

I dont think we ever add functions or call ApplyToFunctions() at "runtime", its just during registration that the function set actually gets mutated, so I still don't see why this is necessary?
Functions identity should (is) be by value anyway, so even if the functions are replaced, they should still compare properly.

Right. I was treating replacement of the shared pointer as a loss of function identity, which is stronger than the contract we need. The snapshot was defending against manually modified definitions rather than a normal execution path. I should be able to remove it and use the existing function identity/comparison mechanisms where the exporter needs to validate a function.

Is this actually used anywhere?

Not in this preparatory PR. The follow-up uses the aggregate version to check that PIVOT's internal list aggregate still resolves to an original registered definition before reconstructing it. The scalar equivalent currently has no caller. I could move it to the next PR.

Could you elaborate on the distinction between a "original registered" function and one that is not? In my mind any function that is part of a function-set in the catalog is by definition a registered function.

By original I meant the objects present when the catalog entry was constructed. That was an implementation distinction, not a meaningful registration distinction. Functions subsequently present in the catalog set are registered too. IsRegisteredFunction is therefore misleading, and I will remove both the method and the snapshot.

I don't really see why FunctionData has to have a InternalKind?

Mh yeah, fair point. I introduced this as a defensive check for the SQL exporter, but it adds a parallel type system to FunctionData. I will remove it and keep bind-data interpretation in function-owned helpers, relying on the existing binder/deserializer contract.

We should just change functions to have a QualifiedName and serialize the qualified name for all functions types - e.g. replace the:

		serializer.WriteProperty(500, "name", function.GetName());
	    ...
		serializer.WritePropertyWithDefault<Identifier>(505, "catalog_name", function.GetCatalogName(), Identifier());
		serializer.WritePropertyWithDefault<Identifier>(506, "schema_name", function.GetSchemaName(), Identifier());

With a

        ...
	    serializer.WriteProperty(507, "qname", function.GetQualifiedName()

Good call! A single QualifiedName across function types is cleaner than adding a separate schema-path property for table functions. I will consolidate the representation and serialization, retaining the legacy fields when targeting older versions and reconstructing the qualified name when reading older serialized plans.

Awesome!

I dont think this is necessary either - named arguments are reordered into positional arguments during binding. The only functions where the argument names matter will store it into their bind data anyway.

If we really want to have the argument names saved so that we can recreate the SQL, we should just do that - store them separately next to the argument expressions themselves. This is what we do in the parsed FunctionExpression, and we could do something similar here too. (maybe not introduce a BoundFunctionArgument, but just have separate:

class BoundFunctionExpression { 
  BoundScalarFunction function;

  vector<unique_ptr<Expression>> argument_exprs;
  vector<Identifier> argument_names;
}

But again, the fundamental problem is that we have functions that alter their input expressions at bind-time. I think that is the real problem we will have to solve before we can go further with this sql-reexport without creating a million special cases and introducing a ton of complexity.

Ok, keeping the argument vectors in sync across optimizations may be difficult as well, depending on what kind of optimizations we do, but at that point, why not just have a typedef unique_ptr<ParsedExpression> scalar_function_unbind_t(const BoundScalarFunction &function) that just gives you the ParsedExpression immediately?, or even a string scalar_function_get_sql_t(const BoundScalarFunction &function)

Yes, this fits the implemented follow-up better than maintaining parallel argument vectors. The exporter already reconstructs children using the current column-binding context, so it could pass those parsed children, the bound function and its bind data to an unbind callback. The function could then construct its own ParsedExpression, while qualification checks and result-type preservation remain in the exporter. I prefer that over returning SQL text, so composition and quoting continue through the existing AST and printer.

So these should be ParsedExpressions then?

Yes. These are retained SQL offsets, not execution expressions, so ParsedExpression is a better representation.

Doesn't the LogicalType::operator== already handle this?

Not for collations: StringTypeInfo::EqualsInternal() explicitly ignores them. VARCHAR and VARCHAR COLLATE nocase therefore compare equal, including when nested inside lists or structs. This helper first checks ordinary type equality, then compares the nested string collations. The exporter needs that stronger comparison to avoid treating expressions with different SQL comparison semantics as interchangeable.

What triggered to make this necessary?

A constant such as 'A' COLLATE nocase exposes the problem. The COLLATE binder updates the expression's return type, but its stored Value still has the original uncollated type. Previously, serialization wrote only that value, and deserialization constructed the constant's return type from it, losing the collation.

This seems like a bigger (separate) problem. Can we open an issue?

Agreed. I will try to extract the annotation-preservation issue into a separate PR with the reduced reproducers and a focused fix. That keeps this PR about retaining reconstruction metadata rather than mixing in broader optimizer correctness changes.

Again, this seems like a bandaid for a bigger problem.

Same problem, yes.

Same here

This seems like it should not be something the FunctionSerializer should be concerned about

This is expression-level optimizer metadata, rather than function identity or bind data. The property is written by BoundFunctionExpression::Serialize, outside FunctionSerializer. It needs to survive serialization because LogicalOperator::Copy() uses that path. The follow-up uses it to distinguish compression-generated expressions from ordinary calls, especially casts. Would you prefer this to live in a more general expression-annotation mechanism?

AFAIK, we never serialize plans back to an older version anyway.

You're right about the normal plan-copy path: it explicitly uses the latest format. I added the fallback after testing an older serialization target, but the callers I found that select older targets are verification paths, not a supported cross-version plan-export workflow. I will remove it.

This seems backwards to me legacy readers do not always rebind the function, in fact most of serialization headaches come from those that don't. If anything we should change how we serialize so that we always rebind.

Yeah, the comment is too broad. This helper forces the no-bind-data path for these particular functions. It does not describe legacy deserialization generally. I will remove it with the older-target fallback.

Always rebinding would be a cleaner direction, but we need to retain the complete binding inputs first. For example, list sorting resolves ordering and collation during binding, so rebinding against changed session defaults can alter the result. That ties into the invocation-preservation work discussed above.

Doing all these copies of child expressions, which in turn can also be "SerializeAsLegacyRebind`'d will have quadratic complexity, which is problematic for deep expression trees right?

Good catch. I will revisit this.

Again, id like this to leak into the general expression tree given that it's a relatively self-contained and specialized optimizer pass.

Maybe this is a nitpick, but it does seem like we would like to avoid copying here. Start and end expressions are probably not that complex - but maybe Its easier if we just preserve the parsed expressions during binding?

Like argument expressions, if we just serialize the "original", we can re-derive the one we use for execution during deserialization (by rebinding) instead of serializing both?

That would avoid serializing both representations. We would need to factor the RANGE binding logic so deserialization can rebuild the endpoint arithmetic and ordering coercions from the retained frame inputs. One constraint from the implemented follow-up is that optimizer changes must remain authoritative: serializing the original syntax must not silently undo a changed endpoint. I will check that contract as part of the refactor rather than simply replacing the serialized execution expressions with the original AST.

I suppose we can store the table ref, but couldn't we also have a callback here instead, e.g. table_function_to_sql() that can be opt-in? I presume most table functions know how to represent themselves as SQL just given their bind data.

The follow-up already has a to_sql callback returning a table-reference AST. The retained TableRef supports its generic fallback, rather than being required by every callback. I will revisit whether that fallback should require explicit opt-in and whether each supported function can reconstruct from its bound inputs instead. Bind data alone won't necessarily retain the original argument expressions.

ah cool, I suppose you only have to actually store it if the to_sql is not set then - or always store it, I guess it doesn't really matter that much.

In general, do these offsets have to be constants or can they be expressions? Like, what is the desired semantics if I do e.g.

SET VARIABLE foobar = 2
SELECT * FROM tbl LIMIT $foobar

Do we re-export the SQL as bound (with foobar resolved to 2), or unbound?

LIMIT and OFFSET can be expressions. This field only records a constant offset that row-group pruning has partly consumed. Expression-valued limits and offsets remain in BoundLimitNode.

For a value already resolved to 2 in the bound plan, the intended export is 2, rather than recovering the original parameter spelling.

Sorry to jump in here, but would this be the time to also consider the use of expressions in the SAMPLE clause? Currently its argument is forced to be a constant. I have had countless cases where I wanted to write

SET VARIABLE n = 100
FROM t SAMPLE $n;

but could not.

Hi @Teggy! Yes, sounds good to me, I'd like that too 👍🏻 I think it deserves a separate PR, though. SAMPLE currently rejects non-constant arguments during parsing, so we'd need to carry the expression into binding and evaluate and validate it there. It's related to what I'm doing here, but it adds functionality beyond retaining the information needed for SQL reconstruction (this PR really is the basis for a proper EXPLAIN (SQL), but 🤫).

Thanks, reasonable. I move that request to its own PR. 👍🏻

P.S. EXPLAIN (SQL): want! 😉

Soon™ 😉