davidm-db commented on code in PR #47403: URL: https://github.com/apache/spark/pull/47403#discussion_r1687861052
########## sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala: ########## @@ -650,14 +657,27 @@ class SparkSession private( private[sql] def sql(sqlText: String, args: Array[_], tracker: QueryPlanningTracker): DataFrame = withActive { val plan = tracker.measurePhase(QueryPlanningTracker.PARSING) { - val parsedPlan = sessionState.sqlParser.parsePlan(sqlText) - if (args.nonEmpty) { - PosParameterizedQuery(parsedPlan, args.map(lit(_).expr).toImmutableArraySeq) - } else { - parsedPlan + val parsedPlan = sessionState.sqlParser.parseScript(sqlText) + parsedPlan match { + case CompoundBody(Seq(singleStmtPlan: SingleStatement), label) if args.nonEmpty => + CompoundBody(Seq(SingleStatement( + PosParameterizedQuery( + singleStmtPlan.parsedPlan, args.map(lit(_).expr).toImmutableArraySeq))), label) + case p => + assert(args.isEmpty, "Named parameters are not supported for batch queries") + p } } - Dataset.ofRows(self, plan, tracker) + + plan match { + case CompoundBody(Seq(singleStmtPlan: SingleStatement), _) => + Dataset.ofRows(self, singleStmtPlan.parsedPlan, tracker) + case _ => + // execute the plan directly if it is not a single statement + val lastRow = executeScript(plan).foldLeft(Array.empty[Row])((_, next) => next) + val attributes = DataTypeUtils.toAttributes(lastRow.head.schema) + Dataset.ofRows(self, LocalRelation.fromExternalRows(attributes, lastRow.toIndexedSeq)) Review Comment: this is a bit of a hard topic at the moment... we decided on this approach for preview for multiple reasons: - this what we will do for initial version of the changes for Spark Connect execution - discussions around multiple topics are still open: - multiple results API - decisions around this will affect single results API as well - multiple results API - from correctness perspective, all statements (including SELECTs) need to be executed eagerly. It makes sense then to have the same behavior with single results API as well - in this case all statements are executed eagerly, but results for all of them except the last one are dropped. - there is still an open discussion whether to include `return` statement - a ton of questions about stored procedures are still an open topic what will probably happen down the line is: - sql() API remains unchanged and only last DataFrame is returned (as you suggested). Requires still a lot of work to support Connect execution, current approach works with Connect already. - [optional] new API to do what we are doing at the moment. - new API for multiple results, stored procedures, execute immediate, etc. since the last part is still an open question, we figured out that we will do a simplest thing that works e2e in all cases and then, after we gather initial feedback from preview, and understand better what we want to do for stored procedures/multiple results, we should actually commit to implement all of the API changes. please let us know your thoughts on this. -- 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: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org