peter-toth commented on code in PR #57516:
URL: https://github.com/apache/spark/pull/57516#discussion_r3649954519
##########
sql/core/src/main/scala/org/apache/spark/sql/classic/SparkSession.scala:
##########
@@ -531,16 +531,19 @@ class SparkSession private(
val parsedPlan = {
// Always parse with parameter context to detect unbound parameter
markers.
// Even if args is empty, we need to detect and reject parameter
markers in the SQL.
- val (paramMap, resolvedParams) = if (args.nonEmpty) {
+ val resolvedParams = if (args.nonEmpty) {
val pMap = args.zipWithIndex.map { case (arg, idx) =>
s"_pos_$idx" -> lit(arg).expr
}.toMap
- (pMap, resolveAndValidateParameters(pMap))
+ resolveAndValidateParameters(pMap)
} else {
- (Map.empty[String, Expression], Map.empty[String, Expression])
+ Map.empty[String, Expression]
}
- val paramContext =
PositionalParameterContext(resolvedParams.values.toSeq)
+ // Look up by the positional key instead of relying on
`resolvedParams.values`:
+ // the map does not preserve insertion order for 5+ entries.
+ val paramContext =
+ PositionalParameterContext(args.indices.map(idx =>
resolvedParams(s"_pos_$idx")))
Review Comment:
**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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
