szehon-ho commented on code in PR #58023:
URL: https://github.com/apache/spark/pull/58023#discussion_r3797950256


##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala:
##########
@@ -3174,4 +3180,16 @@ class JDBCV2Suite extends SharedSparkSession with 
ExplainSuiteHelper {
 
     assertResult(expectedMetadata) { jdbcRdd.getDatabaseMetadata }
   }
+  test("SPARK-58782: null literal in pushed-down aggregate should render as 
NULL not 'null'") {

Review Comment:
   Nit: missing blank line between this and the preceding test.
   
   Also optional: this covers only the aggregate/group-by path, while the title 
mentions predicates too — a `WHERE NULLIF(s, '') IS NULL` variant would cover 
that. `LiteralValueSuite` already pins the contract, so I don't feel strongly.



##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala:
##########
@@ -236,6 +236,12 @@ class JDBCV2Suite extends SharedSparkSession with 
ExplainSuiteHelper {
       batchStmt.addBatch("INSERT INTO \"test\".\"strings_with_nulls\" VALUES 
('a a a')")
       batchStmt.addBatch("INSERT INTO \"test\".\"strings_with_nulls\" VALUES 
(null)")
 
+      batchStmt.addBatch(
+        "CREATE TABLE \"test\".\"null_literal\" (s TEXT(32))")

Review Comment:
   This new table breaks `test("show tables")` around line 1823: it asserts the 
full listing of `h2.test` via `checkAnswer`, which requires exact equality, and 
the expected list currently ends at `strings_with_nulls` — the last table added 
to this suite, whose author had to extend the list for the same reason.
   
   Two ways out:
   - Add `Row("test", "null_literal", false)` to that list. My preference, 
since the dedicated table reads well here.
   - Or drop the table and use `NULLIF(str, 'abc')` over the existing 
`strings_with_nulls`, which exercises the same path with no new fixture.



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

Reply via email to