[SPARK-46179][SQL][FOLLOW UP][TESTS] Remove --ONLY_IF spark from subquery SQL tests to enable PostgreSQL cross-DBMS testing#57172
Conversation
…sts to enable PostgreSQL cross-DBMS testing Remove the `--ONLY_IF spark` guard from the IN/EXISTS subquery golden-file tests and rewrite their temporary-view DDL into the portable `CREATE TEMPORARY VIEW t(cols) AS VALUES ...` form with ANSI-compatible literals, so the same query files can run against PostgreSQL under CrossDbmsQueryTestSuite. Also add debug/error logging to CrossDbmsQueryTestSuite to make per-query cross-DBMS mismatches easier to diagnose. Golden .out files still need to be regenerated with SPARK_GENERATE_GOLDEN_FILES=1.
andylam-db
changed the title
Generated with SPARK_GENERATE_GOLDEN_FILES=1 after removing --ONLY_IF spark and rewriting the temporary-view DDL to the portable CREATE TEMPORARY VIEW t(cols) AS VALUES form. The literal-type changes (dropping S/L/float/BD suffixes) shift column types (smallint->int, float/double->decimal), which is reflected in the regenerated schemas.
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
You need to file a JIRA issue before making it ready, @andylam-db .
andylam-db
changed the title
cloud-fan
left a comment
cloud-fan
left a comment
There was a problem hiding this comment.
0 blocking, 0 non-blocking, 0 nits.
Clean, well-executed test-portability change. No findings.
Verification
Verified the DDL rewrite is result-preserving: after normalizing decimal display (2600 vs 2600.0), a row-by-row comparison across all 19 regenerated results goldens shows zero result-row value changes -- every diff is decimal-scale display or a schema type-name change -- and no query newly errors (20E2BD = 2000 -> 2000.0 is value-preserving). The dropped explicit-type literals narrow some column types (smallint/bigint -> int, float/double -> decimal), but this is correct for CrossDbmsQueryTestSuite: it compares rendered output text and ignores schema (schema = None, CrossDbmsQueryTestSuite.scala:145), so a type change is unobservable unless a row value changes -- and none does. The harness change is logging-only; control flow and assertResult calls are unchanged.
|
The test failures are directly caused by this PR |
These 8 files produce genuine Spark-vs-PostgreSQL divergences when run through PostgresSQLQueryTestSuite: timestamp fractional-second formatting (Spark '...00.0' vs Postgres '...00'), AVG decimal scale (Spark '8.0' vs Postgres '8.0000000000000000'), and implicit-cross-join syntax (FROM t1 JOIN t3 WHERE ... which Postgres rejects). Since --ONLY_IF is a whole-file directive with no per-query granularity, restore it on these files to keep them Spark-only, matching the option documented in PostgresSQLQueryTestSuite. The portable view DDL is retained (harmless). The remaining subquery files still run cross-DBMS against Postgres.
szehon-ho
left a comment
szehon-ho
left a comment
There was a problem hiding this comment.
also, checking, we dont remove test coverage right?
we have adequate AS SELECT * FROM VALUES tests?
|
|
||
| val outputs: Seq[QueryTestOutput] = queries.map { sql => | ||
| val outputs: Seq[QueryTestOutput] = queries.zipWithIndex.map { case (sql, queryIdx) => | ||
| log.debug(s"[${testCase.name}] Executing query #$queryIdx against $DATABASE_NAME: " + |
There was a problem hiding this comment.
are these very useful? (asking if its better to remove)
There was a problem hiding this comment.
Hm, I think it's harmless.
There was a problem hiding this comment.
Was concerned it will spam the logs, but will leave you to.
No, basically we're just adjusting to postgres syntax. |
szehon-ho
changed the title
szehon-ho
changed the title
…query SQL tests to enable PostgreSQL cross-DBMS testing
### What changes were proposed in this pull request?
This PR removes the `--ONLY_IF spark` directive from the IN/EXISTS subquery golden-file tests under `sql/core/src/test/resources/sql-tests/inputs/subquery/` and rewrites their temporary-view DDL from the Spark-specific
```sql
create temporary view t1 as select * from values
("t1a", 6S, 8, 10L, float(15.0), 20D, 20E2BD, ...) as t1(t1a, t1b, ...);
```
form into the portable
```sql
create temporary view t1(t1a, t1b, ...) as values
('t1a', 6, 8, 10, 15.0, 20.0, 2000.0, ...);
```
form, using ANSI-compatible literals (single-quoted strings, no `S`/`L`/`D`/`E2BD` type suffixes). This lets the query files run against PostgreSQL through `CrossDbmsQueryTestSuite`.
### Why are the changes needed?
The subquery test inputs were guarded by `--ONLY_IF spark`, so they never exercised the cross-DBMS path. Their view DDL relied on Spark-only syntax (typed literal suffixes, `select * from values ... as t(...)`) that PostgreSQL rejects. Making the DDL portable lets these tests validate Spark's subquery semantics against PostgreSQL as an independent oracle, catching correctness divergences.
### Does this PR introduce _any_ user-facing change?
No. Test-only change.
### How was this patch tested?
Existing golden-file tests. The golden `.out` files are regenerated with `SPARK_GENERATE_GOLDEN_FILES=1` and the PostgreSQL cross-DBMS run is exercised via `CrossDbmsQueryTestSuite`.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Anthropic)
Closes #57172 from andylam-db/andylam-db/postgres-subquery-tests.
Authored-by: Andy Lam <andy.lam@databricks.com>
Signed-off-by: Szehon Ho <szehon.apache@gmail.com>
(cherry picked from commit 5e3509f)
Signed-off-by: Szehon Ho <szehon.apache@gmail.com>
|
Merged to master, 4.x, thanks! |
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Thank you for finalizing and landing this, @andylam-db and all.
4 participants
What changes were proposed in this pull request?
This PR removes the
--ONLY_IF sparkdirective from the IN/EXISTS subquery golden-file tests undersql/core/src/test/resources/sql-tests/inputs/subquery/and rewrites their temporary-view DDL from the Spark-specificform into the portable
form, using ANSI-compatible literals (single-quoted strings, no
S/L/D/E2BDtype suffixes). This lets the query files run against PostgreSQL throughCrossDbmsQueryTestSuite.Why are the changes needed?
The subquery test inputs were guarded by
--ONLY_IF spark, so they never exercised the cross-DBMS path. Their view DDL relied on Spark-only syntax (typed literal suffixes,select * from values ... as t(...)) that PostgreSQL rejects. Making the DDL portable lets these tests validate Spark's subquery semantics against PostgreSQL as an independent oracle, catching correctness divergences.Does this PR introduce any user-facing change?
No. Test-only change.
How was this patch tested?
Existing golden-file tests. The golden
.outfiles are regenerated withSPARK_GENERATE_GOLDEN_FILES=1and the PostgreSQL cross-DBMS run is exercised viaCrossDbmsQueryTestSuite.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Anthropic)