Fix SQL DML Clone correctness bugs

[snaumenko-st]

TL;DR;

• SqlUpdate.Clone: copy Limit from t.limit (was t.where).
• SqlSelect.Clone: deep-clone Limit/Offset via context.
• SqlConcat/SqlRow.Clone: register in NodeMapping via GetOrAdd.
• SqlJoinHint/SqlForceJoinOrderHint.Clone: propagate clone context to inner tables.
• SqlInsertValuesCollection.Clone: pass ctx to row clones for cross-row dedup.

---

1. SqlUpdate.Clone — Limit was being copied from Where

File: Orm/Xtensive.Orm/Sql/Dml/Statements/SqlUpdate.cs

Before

    internal override SqlUpdate Clone(SqlNodeCloneContext context) =>
      context.GetOrAdd(this, static (t, c) => {
        var clone = new SqlUpdate();
        // ...
        if (t.where is not null)
          clone.Where = t.where.Clone(c);
        if (t.limit is not null)
          clone.Limit = t.where.Clone(c);   // <-- BUG: clones t.where again

What was wrong

A copy/paste mistake: when t.limit was non-null the clone copied t.where into Limit. Two consequences:

• Correctness: the cloned SqlUpdate had its Limit pointing at a clone of the WHERE expression — which would either fail validation (SqlValidator.EnsureIsLimitOffsetArgument is invoked from the Limit setter) or produce a logically incorrect statement when the WHERE happens to be a numeric expression.
• NRE risk: if t.where was null but t.limit was not, this dereferenced null.
• Wasted work: even when it didn't crash, it cloned where twice.

Fix

Use t.limit.Clone(c), mirroring how every other field is cloned.

        if (t.limit is not null)
          clone.Limit = t.limit.Clone(c);

---

2. SqlSelect.Clone — Limit and Offset were shared by reference

File: Orm/Xtensive.Orm/Sql/Dml/Statements/SqlSelect.cs

Before

    internal override SqlSelect Clone(SqlNodeCloneContext context) =>
      context.GetOrAdd(this, static (t, c) => {
        SqlSelect clone = new(t.From?.Clone(c));
        // columns/groupBy/where/having/orderBy all use .Clone(c)
        clone.Distinct = t.Distinct;
        clone.Limit = t.Limit;     // <-- SHARED REFERENCE
        clone.Offset = t.Offset;   // <-- SHARED REFERENCE
        clone.Lock = t.Lock;
        clone.Comment = t.Comment?.Clone(c);

What was wrong

Every other expression-typed field of SqlSelect is deep-cloned through the context (t.where.Clone(c), t.having.Clone(c), etc.). Limit and Offset were the only SqlExpression-typed members assigned directly. That breaks the deep-clone invariant of Clone(SqlNodeCloneContext): mutating the cloned tree's Limit/Offset would mutate the source tree's, and vice-versa.

This is also subtle — Limit is often a literal like SqlDml.Literal(10), so for many queries no one ever notices. But when Limit/Offset is a parameter binding, computed expression, or anything that gets re-walked or post-processed, the alias becomes a real bug (and breaks SqlNodeCloneContext's NodeMapping invariant — that mapping should let visitors find the cloned counterpart of any source node).

Fix

Clone via the active context, preserving null semantics:

        clone.Limit = t.Limit?.Clone(c);
        clone.Offset = t.Offset?.Clone(c);

The setters validate via SqlValidator.EnsureIsLimitOffsetArgument, which still works because Clone returns the same shape of expression.

---

3. SqlConcat.Clone — never registered itself in NodeMapping

File: Orm/Xtensive.Orm/Sql/Dml/Expressions/SqlConcat.cs

Before

    internal override SqlConcat Clone(SqlNodeCloneContext context)
    {
      if (context.NodeMapping.TryGetValue(this, out var value)) {
        return (SqlConcat)value;
      }

      var expressionsClone = new SqlExpression[expressions.Count];
      int i = 0;
      foreach (var e in expressions)
        expressionsClone[i++] = e.Clone(context);

      var clone = new SqlConcat(expressionsClone);
      return clone;
    }

What was wrong

The method checks NodeMapping for an existing clone, but never inserts the new clone. The TryGetValue branch was effectively dead code — unless some external caller pre-registered this SqlConcat, it would always miss. Net effect:

• Every SqlConcat reached more than once during a single clone walk was rebuilt from scratch (more allocations, deeper recursion).
• Deduplication semantics differed from every other SqlExpressionList subclass that uses context.GetOrAdd.

Fix

Route through the standard helper, which both checks the mapping and records the new clone:

    internal override SqlConcat Clone(SqlNodeCloneContext context) =>
      context.GetOrAdd(this, static (t, c) => {
        var source = t.expressions;
        var expressionsClone = new SqlExpression[source.Count];
        for (int i = 0; i < source.Count; i++)
          expressionsClone[i] = source[i].Clone(c);
        return new SqlConcat(expressionsClone);
      });

Two side benefits beyond the bug fix:

• static lambda + GetOrAdd<T> → no captured-closure allocation.
• Indexed for over IReadOnlyList<SqlExpression> avoids the IEnumerator<SqlExpression> heap allocation that foreach would have produced (since expressions is typed as the interface, not a concrete List<>).

---

4. SqlRow.Clone — same TryGet-without-register bug, plus LINQ allocation

File: Orm/Xtensive.Orm/Sql/Dml/Expressions/SqlRow.cs

Before

using System;
using System.Collections.Generic;
using System.Linq;
using Xtensive.Core;

namespace Xtensive.Sql.Dml
{
  [Serializable]
  public class SqlRow: SqlExpressionList
  {
    internal override SqlRow Clone(SqlNodeCloneContext context)
    {
      return (context.TryGet(this) as SqlRow) ?? new(expressions.Select(e => e.Clone(context)).ToArray());
    }

What was wrong

Same dead-code problem as SqlConcat: the TryGet(this) lookup will always return null because nothing inserts the new SqlRow into NodeMapping. So:

• TryGet runs every time uselessly — and worse, TryGet<T> allocates internally if the cast returns null (it doesn't actually, but the boxed dictionary lookup is wasted).
• Every shared SqlRow re-clones from scratch.
• expressions.Select(...).ToArray() allocates a Select-iterator and a Func<SqlExpression, SqlExpression> (closure over context) per clone, on top of the result array.

Fix

Same pattern as SqlConcat, plus drops the now-unused using System.Linq;:

using System;
using System.Collections.Generic;
using Xtensive.Core;

namespace Xtensive.Sql.Dml
{
  [Serializable]
  public class SqlRow: SqlExpressionList
  {
    internal override SqlRow Clone(SqlNodeCloneContext context) =>
      context.GetOrAdd(this, static (t, c) => {
        var source = t.expressions;
        var expressionsClone = new SqlExpression[source.Count];
        for (int i = 0; i < source.Count; i++)
          expressionsClone[i] = source[i].Clone(c);
        return new SqlRow(expressionsClone);
      });

---

5. SqlJoinHint.Clone — dropped the clone context

File: Orm/Xtensive.Orm/Sql/Dml/Hints/SqlJoinHint.cs

Before

    internal override SqlJoinHint Clone(SqlNodeCloneContext context) =>
      context.GetOrAdd(this, static (t, c) => new(t.Method, (SqlTable) t.Table.Clone()));

What was wrong

t.Table.Clone() (no c) is the parameterless overload on SqlNode that creates a fresh SqlNodeCloneContext internally. That means:

• The hint's table reference is cloned in isolation from the surrounding statement clone.
• The outer statement's NodeMapping doesn't see this fresh table, so any subsequent reference to the same SqlTable during the outer clone will produce a second, divergent clone — i.e. the cloned join hint references a different SqlTable instance than the join in the FROM/JOIN tree it's annotating.

In query optimizer hint resolution that lookup-by-reference is exactly what the translator relies on, so this was a real correctness bug for any cloned statement that carried a SqlJoinHint.

Fix

Pass the surrounding context through:

    internal override SqlJoinHint Clone(SqlNodeCloneContext context) =>
      context.GetOrAdd(this, static (t, c) => new(t.Method, (SqlTable) t.Table.Clone(c)));

Now the table will be deduplicated against any other reference to the same source table within the same outer clone.

---

6. SqlForceJoinOrderHint.Clone — same context-drop, plus Select(...).ToArray()

File: Orm/Xtensive.Orm/Sql/Dml/Hints/SqlForceJoinOrderHint.cs

Before

using System;
using System.Linq;
using System.Collections.Generic;
using Xtensive.Core;

namespace Xtensive.Sql.Dml
{
  [Serializable]
  public class SqlForceJoinOrderHint : SqlHint
  {
    public IReadOnlyList<SqlTable> Tables { get; }

    internal override SqlForceJoinOrderHint Clone(SqlNodeCloneContext context) =>
      context.GetOrAdd(this, static (t, c) =>
        new SqlForceJoinOrderHint(t.Tables?.Select(table => (SqlTable) table.Clone()).ToArray()));

What was wrong

Same fundamental issue as SqlJoinHint, multiplied by the number of tables in the order hint:

• Each table.Clone() (no c) detaches that table from the outer mapping.
• t.Tables?.Select(...).ToArray() adds the standard LINQ allocations: a Select iterator and a Func closure.
• The ?. short-circuit was preserved by emitting the parameterless SqlForceJoinOrderHint() ctor when Tables is null.

Fix

Loop with the shared context, pre-sized array, no LINQ; drop the using System.Linq;:

using System;
using System.Collections.Generic;
using Xtensive.Core;

namespace Xtensive.Sql.Dml
{
  [Serializable]
  public class SqlForceJoinOrderHint : SqlHint
  {
    public IReadOnlyList<SqlTable> Tables { get; }

    internal override SqlForceJoinOrderHint Clone(SqlNodeCloneContext context) =>
      context.GetOrAdd(this, static (t, c) => {
        if (t.Tables is null)
          return new SqlForceJoinOrderHint();
        var source = t.Tables;
        var tablesClone = new SqlTable[source.Count];
        for (int i = 0; i < source.Count; i++)
          tablesClone[i] = (SqlTable) source[i].Clone(c);
        return new SqlForceJoinOrderHint(tablesClone);
      });

Now every table inside the order hint dedupes against the FROM-clause tables in the cloned statement.

---

7. SqlInsertValuesCollection.Clone — fresh context per row

File: Orm/Xtensive.Orm/Sql/Dml/Collections/SqlInsertValuesCollection.cs

Before

    internal SqlInsertValuesCollection Clone(SqlNodeCloneContext ctx)
    {
      var clone = new SqlInsertValuesCollection();

      if (rows.Count == 0) {
        return clone;
      }

      var clonedList = new List<SqlColumn>(columns.Count);
      foreach (var oldColumn in columns) {
        clonedList.Add((SqlColumn) ctx.NodeMapping[oldColumn]);
      }
      clone.columns = clonedList;

      clone.rows = new List<SqlRow>(rows.Count);
      foreach(var oldRow in rows) {
        clone.rows.Add((SqlRow) oldRow.Clone());   // <-- new context per row
      }

      return clone;
    }

What was wrong

The columns list was cloned correctly through the supplied ctx (it even reads ctx.NodeMapping[oldColumn] directly, depending on the outer statement having registered the columns). But each row was cloned via the parameterless oldRow.Clone(), which:

• Spins up a fresh SqlNodeCloneContext per row. The struct's backing Dictionary<SqlNode, SqlNode> is allocated for every row in the INSERT. For multi-row INSERTs this is N extra dictionaries.
• Defeats cross-row deduplication. If two rows share a sub-expression instance (a parameter, literal, or computed expression), they'd be cloned independently, producing diverging expression trees in the cloned statement and breaking any outer-statement invariant that holds across rows.
• Defeats column ↔ row dedup. After the fix, an ISqlLValue referenced both by columns and inside a row's expressions resolves to the same cloned node. Before, it would be split.
• Type cast was redundant. SqlRow.Clone(...) already returns SqlRow; the (SqlRow) cast was for the parameterless Clone() which returns SqlNode.

Fix

Pass ctx through, drop the cast:

      clone.rows = new List<SqlRow>(rows.Count);
      foreach(var oldRow in rows) {
        clone.rows.Add(oldRow.Clone(ctx));
      }

Combined with the SqlRow.Clone fix above (which now actually registers in NodeMapping), this means: shared SqlRow instances across the INSERT statement now clone exactly once, and shared sub-expressions inside those rows now also dedupe within the same outer clone.

---

Cross-cutting themes

A few invariants the original code violated that the fixes restore:

1. Clone(SqlNodeCloneContext) must always traverse with the supplied context. The parameterless Clone() is for "I want a brand-new isolated copy of this subtree" — never for a sub-call inside another Clone(ctx). Both hint classes and SqlInsertValuesCollection were violating this.

2. Clone(SqlNodeCloneContext) must register the produced clone in NodeMapping before returning. Otherwise the dedup test at the start of every other Clone is meaningless and you can produce divergent clones of the same source node. The canonical pattern is context.GetOrAdd(this, static (t, c) => …). SqlConcat and SqlRow were violating this.

3. All SqlExpression-typed fields in a statement should be deep-cloned, not assigned by reference. SqlSelect.Limit/Offset were the only fields breaking this rule.

4. The Tier-1 perf wins are incidental. Static lambdas avoid closure allocations; pre-sized arrays + indexed for over IReadOnlyList<> avoid Select/ToArray and enumerator boxing. These are small per call, but Clone is on the cached-corrector path so they multiply.

---

Made-with the help of AI tooling