[SPARK-58341][SQL] Fix wrong results with 5+ positional parameters in sql

[dongjoon-hyun]

What changes were proposed in this pull request?

This PR fixes a correctness issue where spark.sql binds 5 or more positional parameters in the wrong order. The resolved parameters were collected via resolvedParams.values.toSeq from a Map keyed by _pos_<idx>, but Scala's immutable Map does not preserve insertion order for 5+ entries. This PR looks them up by the positional key instead, in both affected places:

• classic.SparkSession.sql(sqlText, args: Array[_], tracker)
• SparkConnectPlanner.buildParameterContext (pos_arguments)

Why are the changes needed?

Since Apache Spark 4.1.0 (SPARK-53573), queries with 5+ positional parameters
silently return wrong results in both the classic API and Spark Connect.

• [SPARK-53573][SQL] Use Pre-processor for generalized parameter marker handling #52334

BEFORE (Spark 4.2.0)

$ bin/spark-shell                                                                          
WARNING: Using incubator modules: jdk.incubator.vector
Using Spark's default log4j profile: org/apache/spark/log4j2-defaults.properties
Setting default log level to "WARN".
To adjust logging level use sc.setLogLevel(newLevel). For SparkR, use setLogLevel(newLevel).
Welcome to
      ____              __
     / __/__  ___ _____/ /__
    _\ \/ _ \/ _ `/ __/  '_/
   /___/ .__/\_,_/_/ /_/\_\   version 4.2.0
      /_/
         
Using Scala version 2.13.18 (OpenJDK 64-Bit Server VM, Java 21.0.12)
Type in expressions to have them evaluated.
Type :help for more information.
26/07/24 15:46:06 WARN NativeCodeLoader: Unable to load native-hadoop library for your platform... using builtin-java classes where applicable
Spark context Web UI available at http://localhost:4040
Spark context available as 'sc' (master = local[*], app id = local-1784933166639).
Spark session available as 'spark'.

scala> spark.sql("SELECT ?, ?, ?, ?, ?, ?", Array(1, 2, 3, 4, 5, 6)).collect()
val res0: Array[org.apache.spark.sql.Row] = Array([6,2,5,3,4,1])

AFTER (this PR)

$ bin/spark-shell
WARNING: Using incubator modules: jdk.incubator.vector
Using Spark's default log4j profile: org/apache/spark/log4j2-defaults.properties
Setting default log level to "WARN".
To adjust logging level use sc.setLogLevel(newLevel). For SparkR, use setLogLevel(newLevel).
Welcome to
      ____              __
     / __/__  ___ _____/ /__
    _\ \/ _ \/ _ `/ __/  '_/
   /___/ .__/\_,_/_/ /_/\_\   version 5.0.0-SNAPSHOT
      /_/
         
Using Scala version 2.13.18 (OpenJDK 64-Bit Server VM, Java 21.0.12)
Type in expressions to have them evaluated.
Type :help for more information.
26/07/24 15:48:50 WARN NativeCodeLoader: Unable to load native-hadoop library for your platform... using builtin-java classes where applicable
Spark context Web UI available at http://localhost:4040
Spark context available as 'sc' (master = local[*], app id = local-1784933331484).
Spark session available as 'spark'.

scala> spark.sql("SELECT ?, ?, ?, ?, ?, ?", Array(1, 2, 3, 4, 5, 6)).collect()
val res0: Array[org.apache.spark.sql.Row] = Array([1,2,3,4,5,6])

Does this PR introduce any user-facing change?

Yes, this is a correctness bug fix of the released versions 4.1.0, 4.1.1, 4.1.2, 4.1.3 and 4.2.0.

How was this patch tested?

Pass the CIs with the newly added test case.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Fable 5

[dongjoon-hyun]

• cc @srielau , @dtenedor , @cloud-fan from the original PR.
  • [SPARK-53573][SQL] Use Pre-processor for generalized parameter marker handling #52334
• cc @HyukjinKwon and @HeartSaVioR because this is a correctness issue for Apache Spark 4.3.0 too.
• And, cc @MaxGekk , @viirya , @sunchao , @peter-toth , too.

[uros-b]

LGTM

[viirya]

Clean, well-targeted correctness fix. The root cause is exactly right: resolveAndValidateParameters returns a plain immutable Map keyed by _pos_<idx>, and Scala's Map only preserves insertion order up to 4 entries (Map1..Map4) before switching to a hash-ordered HashMap at 5+, so .values.toSeq scrambles the binding order — which is precisely why the bug only shows up at 5+ params. Looking up by _pos_<idx> in index order is the correct fix, and since the map is keyed _pos_0.._pos_(n-1), args.indices / paramList.indices hits every key with no missing-key risk.

I checked bug-class completeness independently and the fix is complete:

• These two (SparkSession.sql and SparkConnectPlanner.buildParameterContext) are the only sites using the buggy build-_pos_-map-then-.values.toSeq pattern.
• The third positional path — SparkSession.sql(sqlText, args, paramNames, tracker) (the EXECUTE IMMEDIATE / hybrid one) — was already doing the right thing (args.indices.map { idx => resolvedParams(name) }), so this PR actually brings the two divergent sites back in line with the idiom already used there. Nice consistency win.
• Client-side positional args (e.g. PySpark Connect) come from an ordered repeated proto field, not a Scala Map, so they're unaffected.

The tests are good — the classic case using non-commutative ops (? + ?, ? * ?, ? - ?) is a nice touch, since it fails on a wrong order even if the values happened to round-trip.

Two very minor, non-blocking notes:

1. The tests use 6 params; a 5-param case would exercise the exact Map4->HashMap boundary, though 6 is already past it and sufficient as a regression guard.
2. The fix relies on the _pos_0.._pos_(n-1) keys being contiguous, which holds because the same idx range builds and reads the map in one place — just an implicit coupling to keep in mind if the key format ever changes.

LGTM.

[dongjoon-hyun]

Thank you, @uros-b and @viirya .

[peter-toth]

Thanks for the PR, @dongjoon-hyun!

Independently confirmed the root cause and the fix. resolveAndValidateParameters returns a plain immutable Map keyed by _pos_<idx>, and Scala's immutable Map switches from the order-preserving Map1..Map4 to a HashMap at 5 entries, so .values.toSeq iterates in hash order for 5+ params. Rebuilding the ordered Seq via args.indices.map(idx => resolvedParams(s"_pos_$idx")) is correct: every _pos_$idx key is guaranteed present (same loop populates the map) and the indices are distinct, so no missing-key or dup risk. Both fixed spots now match the already-correct siblings (the EXECUTE IMMEDIATE hybrid path at SparkSession.scala:668-676 and the deprecated pos_args literal branch). Tests cover both the classic and Connect paths at the 5+ regime. LGTM.

Alternatives

• 1. Fix the order loss at the source: the "build _pos_ map -> resolve -> rebuild ordered Seq by key" dance now lives at two callsites because resolveAndValidateParameters hands back an order-losing Map. Having it return an order-preserving type (ListMap, or a positional Seq variant) would make .values.toSeq safe for every current and future caller. The surgical per-callsite fix here is the right call for this PR though -- it's minimal and clean to backport to 4.1/4.2/4.3 -- so this is just a follow-up-ticket thought, not a blocker. [inline: sql/core/src/main/scala/org/apache/spark/sql/classic/SparkSession.scala:546]

[peter-toth]

Finding 1. The fix is correct as written. Just flagging a possible follow-up: the underlying trap is that resolveAndValidateParameters (SparkSession.scala:469) returns a plain immutable Map, which loses insertion order at 5+ entries -- so every caller that wants positional order has to rebuild it from the _pos_<idx> keys (here and in SparkConnectPlanner.buildParameterContext). Fixing it at the source -- return a scala.collection.immutable.ListMap (insertion-order-preserving) or expose a positional Seq -- would make .values.toSeq order-safe for all callers and remove the per-callsite rebuild, protecting future ones from the same bug. Not for this PR: the surgical fix is the safer, easy-to-backport choice; worth a separate follow-up ticket if you agree.

[sunchao]

Thanks for the fix. I reviewed both the classic SQL and Spark Connect positional-parameter paths and confirmed that looking up each resolved _pos_<idx> in its original index order fixes the five-or-more-parameter correctness bug without depending on immutable Map iteration order.

The named, hybrid, legacy, SQL-scripting, and deprecated Spark Connect argument paths retain their existing behavior. The new classic and Connect regression tests exercise the affected case, including order-sensitive arithmetic, and both checks on the reviewed commit are passing.

Returning an order-preserving collection from the resolver can be considered separately, but is not necessary for this focused, backportable correctness fix. LGTM.

[dongjoon-hyun]

Thank you, @sarutak , @peter-toth , @sunchao .

[dongjoon-hyun]

Merge Summary:

• merged into master b910d6f
• merged into branch-4.x 3b85051
• merged into branch-4.2 fc6979c
• merged into branch-4.1 e47a13c

Posted by merge_spark_pr.py