miland-db commented on code in PR #49372:
URL: https://github.com/apache/spark/pull/49372#discussion_r1907265070


##########
sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecution.scala:
##########
@@ -42,32 +45,53 @@ class SqlScriptingExecution(
     val ctx = new SqlScriptingExecutionContext()
     val executionPlan = interpreter.buildExecutionPlan(sqlScript, args, ctx)
     // Add frame which represents SQL Script to the context.
-    ctx.frames.addOne(new 
SqlScriptingExecutionFrame(executionPlan.getTreeIterator))
+    ctx.frames.append(new 
SqlScriptingExecutionFrame(executionPlan.getTreeIterator))
     // Enter the scope of the top level compound.
     // We don't need to exit this scope explicitly as it will be done 
automatically
     // when the frame is removed during iteration.
     executionPlan.enterScope()
     ctx
   }
 
-  private var current: Option[DataFrame] = getNextResult
+  private var current: Option[DataFrame] = None
 
-  override def hasNext: Boolean = current.isDefined
+  /**
+   * Advances through the script and executes statements until a result 
statement or
+   * end of script is encountered.
+   *
+   * To know if there is result statement available, the hasNext needs to 
advance through
+   * script and execute statements until the result statement or end of script 
is encountered.
+   * For that reason hasNext must be called only once before each `next()` 
invocation,
+   * and the returned result must be executed before subsequent calls. 
Multiple calls without
+   * executing the intermediate results will lead to incorrect behavior.
+   *
+   * @return True if a result statement is available, False otherwise.
+   */
+  override def hasNext: Boolean = {
+    current = getNextResult
+    current.isDefined
+  }
 
+  /**
+   * Returns the next result statement from the script.
+   * Multiple consecutive calls without calling `hasNext()` would return the 
same result statement.

Review Comment:
   Because of the nature of contract between the **caller** and 
**SqlScriptingExecution API** which is to execute returned statement before 
proceeding with iteration, we can't advance further before first executing the 
returned statement.
   
   Advancing in the `next` method immediately after returning the **current 
statement** would break the contract. 
   Advancing to the next result statement or end of script has to be done after 
current result statement is executed. For that reason `next` can't be the one 
advancing the iteration. Here is the example of the code that would **not** 
work correctly:
   ```
   override def hasNext(): Boolean = current.isDefined
   
   override def next(): DataFrame = {
     val tmp = current
     current = getNextResult
     return tmp
   }
   ```
   
   The idea is to **enforce correct usage** of SqlScriptingExecution API 
through the future **PR review process**, and keep the behavior of `hasNext` 
and `next` **well documented**.
   
   cc. @davidm-db @dejankrak-db 



-- 
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

Reply via email to