[jira] [Commented] (SPARK-27772) SQLTestUtils Refactoring

["Hyukjin Kwon (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/SPARK-27772?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16843596#comment-16843596 ] 

Hyukjin Kwon commented on SPARK-27772:
--------------------------------------

Can you describe one actual case that {{withTempView}} can be problematic?  I haven't met any actual case before.

> SQLTestUtils Refactoring
> ------------------------
>
>                 Key: SPARK-27772
>                 URL: https://issues.apache.org/jira/browse/SPARK-27772
>             Project: Spark
>          Issue Type: Improvement
>          Components: SQL
>    Affects Versions: 3.0.0
>            Reporter: William Wong
>            Priority: Minor
>
> The current `SQLTestUtils` created many `withXXX` utility functions to clean up tables/views/caches created for testing purpose. Some of those `withXXX` functions ignore certain exceptions, like `NoSuchTableException` in the clean up block (ie, the finally block). 
>  
> {code:java}
> /**
>  * Drops temporary view `viewNames` after calling `f`.
>  */
> protected def withTempView(viewNames: String*)(f: => Unit): Unit = {
>   try f finally {
>     // If the test failed part way, we don't want to mask the failure by failing to remove
>     // temp views that never got created.
>     try viewNames.foreach(spark.catalog.dropTempView) catch {
>       case _: NoSuchTableException =>
>     }
>   }
> }
> {code}
> Maybe it is not the best approach. Because it is hard to anticipate what exception should or should not be ignored.  
>  
> We may hit similar scenario with Java's `try-with-resources` statement. Java does not mask exception throws in the try block with any exception caught in the 'close()' statement. Exception caught in the 'close()' statement would add as a suppressed exception instead. IMHO, it is a better approach.  
>  
> Therefore, I proposed to standardise those 'withXXX' function with following `withFinallyBlock` function, which does something similar to Java's try-with-resources statement. 
> {code:java}
> /**
> * Drops temporary view `viewNames` after calling `f`.
> */
> protected def withTempView(viewNames: String*)(f: => Unit): Unit = {
>   withFinallyBlock(f)(viewNames.foreach(spark.catalog.dropTempView))
> }
> /**
>  * Executes the given tryBlock and then the given finallyBlock no matter whether tryBlock throws
>  * an exception. If both tryBlock and finallyBlock throw exceptions, the exception thrown
>  * from the finallyBlock with be added to the exception thrown from tryBlock as a
>  * suppress exception. It helps to avoid masking the exception from tryBlock with exception
>  * from finallyBlock
>  */
> private def withFinallyBlock(tryBlock: => Unit)(finallyBlock: => Unit): Unit = {
>   var fromTryBlock : Throwable = null
>   try tryBlock catch {
>     case cause : Throwable =>
>       fromTryBlock = cause
>       throw cause
>   } finally {
>     if (fromTryBlock != null) {
>       try finallyBlock catch {
>         case fromFinallyBlock : Throwable =>
>           fromTryBlock.addSuppressed(fromFinallyBlock)
>           throw fromTryBlock
>       }
>     } else {
>       finallyBlock
>     }
>   }
> }
> {code}
>  



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@spark.apache.org
For additional commands, e-mail: issues-help@spark.apache.org