spark-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [spark] peter-toth commented on a change in pull request #26028: [SPARK-29359][SQL][TESTS] Better exception handling in (SQL|ThriftServer)QueryTestSuite
Date Sat, 05 Oct 2019 07:43:12 GMT
peter-toth commented on a change in pull request #26028: [SPARK-29359][SQL][TESTS] Better exception
handling in (SQL|ThriftServer)QueryTestSuite
URL: https://github.com/apache/spark/pull/26028#discussion_r331737306
 
 

 ##########
 File path: sql/core/src/test/resources/sql-tests/results/ansi/interval.sql.out
 ##########
 @@ -290,7 +290,7 @@ interval 4 weeks 2 days
 -- !query 22
 select 30 day day
 -- !query 22 schema
-struct<>
+
 
 Review comment:
   Thanks @dongjoon-hyun for the review, let me try to convince you that these changes make
sense, but if you still disagree just let me know and I will drop them.
   
   The only cases where I replaced the expected schema from `struct<>` to nothing are
those where some error occurs and an exception is thrown. In those cases there is no data
returned, so there is no schema at all, not even an empty struct.
   ```
   -- !query 22
   select 30 day day
   -- !query 22 schema
   
   -- !query 22 output
   org.apache.spark.sql.catalyst.parser.ParseException
   ```
   IMHO `struct<>` makes sense where a statement was successful but data returned is
empty and there are no columns in it. In those cases I left the expected output intact.
   ```
   -- !query 0
   CREATE OR REPLACE TEMPORARY VIEW view1 AS SELECT 2 AS i1
   -- !query 0 schema
   struct<>
   -- !query 0 output
   ```
   Empty expected schema is also useful to easily recognize a statement that ended up in an
error (otherwise we probably need to check the output for containing `exception` which seems
less elegant).
   I utilized empty expected schema to fix a sorting issue in `ThriftServerQueryTestSuite`:
https://github.com/apache/spark/pull/26028/files#diff-b3ea3021602a88056e52bf83d8782de8R147,
but here might be other cases in the future where this could help.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

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


Mime
View raw message