[SPARK-46179][SQL] Add CrossDbmsQueryTestSuites, which runs other DBMS against golden files, starting with Postgres #44084

andylam-db · 2023-11-29T21:56:13Z

What changes were proposed in this pull request?

Create CrossDbmsQueryTestSuite, which extends SQLQueryTestHelper and DockerIntegrationSuite. CrossDbmsQueryTestSuite is a trait class that allows testing golden files against other DBMS.

PostgreSQLQueryTestSuite is an implementation of CrossDbmsQueryTestSuite. For starters, sql files in the subquery sql-tests are automatically opted into this test. In this PR, all files except for exists-having.sql are ignored, otherwise this PR would have 10K+ line changes (I would like to do that in the next PR, if possible). I had to change the syntax for view creation in exists-having.sql slightly, and this is reflected in the analyzer-results file, but crucially, the query output (in the results file) are not changed.

Note that this will not be applicable to many of the current sql tests we have due to:

Incompatible SQL syntax between spark sql and postgres.
Incompatible data types.
Difference in numerical precision with doubles.
Missing functions in either system.
Test files with specific configs, such as ANSI, count bug etc.

Why are the changes needed?

For correctness checking of our SQLQueryTestSuites, we want to run SQLQueryTestSuites with Postgres as a reference DBMS. This can be easily extensible to other DBMS.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

This is a test-related PR, does not affect system behaviors.

Was this patch authored or co-authored using generative AI tooling?

No.

dtenedor

Thanks for adding this, it will be useful to increase test coverage!

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

sql/core/src/test/scala/org/apache/spark/sql/crossdbms/JdbcConnection.scala

sql/core/src/test/scala/org/apache/spark/sql/crossdbms/CrossDbmsQueryTestSuite.scala

sql/core/src/test/resources/sql-tests/inputs/postgres-crosstest/sqllogictest-select1.sql

This reverts commit bef8011.

sql/core/src/test/scala/org/apache/spark/sql/crossdbms/CrossDbmsQueryTestSuite.scala

andylam-db · 2024-01-03T02:03:32Z

Then shall we always let Spark generate golden files? What we need to do is to let other DBMS run the query and compare results with golden files (exclude the schema part).

The reason why we wanted to use DBMS to generate golden files is because we didn't want the CI/CD/developer to have to install some version of postgres/other DBMS just to run tests.

cloud-fan · 2024-01-03T02:28:58Z

because we didn't want the CI/CD/developer to have to install some version of postgres/other DBMS just to run tests.

Isn't it better to trust CI/CD to install the proper version of DBMS and run the tests, instead of humans? AFAIK we are already doing it to run tests for JDBC data sources. We already have postgres tests in https://github.com/apache/spark/blob/master/connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/PostgresIntegrationSuite.scala

andylam-db · 2024-01-03T23:29:01Z

We are already doing it to run tests for JDBC data sources

@cloud-fan I didn't know about those tests! I made the changes to add this as an integration test instead, as you suggested. The two tests (SQLQueryTestSuite and PostgresIntegrationSuite) are a bit disconnected (reasons below). But I think they can be improved in the future. I would like to push this PR as is, if possible.

It's maybe a bit weird that a test in spark/connector uses/affects test files in spark/core.
There's some code duplication to read golden files, but I think not too much.

cloud-fan · 2024-01-04T02:51:27Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestHelper.scala

    }
  }
+
+  /** A test case. */


I assume these are just code move around, no actual changes?

Yes, that's correct.

cloud-fan · 2024-01-04T02:55:17Z

It's maybe a bit weird that a test in spark/connector uses/affects test files in spark/core.

I think it's OK. We have JDBC tests in spark/connector that depend on tests in sql/core

There's some code duplication to read golden files, but I think not too much.

Is there a way to refactor it?

andylam-db · 2024-01-04T07:18:33Z

Is there a way to refactor it?

I tried to move as much code as possible to the trait SQLQueryHelper. Duplicated code is 20+ lines, I don't think it's an issue.

andylam-db · 2024-01-04T17:15:20Z

@cloud-fan Can we merge?

cloud-fan · 2024-01-05T03:36:56Z

thanks, merging to master!

…S against golden files with other DBMS, starting with Postgres ### What changes were proposed in this pull request? Create `CrossDbmsQueryTestSuite`, which extends `SQLQueryTestHelper` and `DockerIntegrationSuite`. `CrossDbmsQueryTestSuite` is a trait class that allows testing golden files against other DBMS. `PostgreSQLQueryTestSuite` is an implementation of `CrossDbmsQueryTestSuite`. For starters, sql files in the subquery sql-tests are automatically opted into this test. In this PR, all files except for `exists-having.sql` are ignored, otherwise this PR would have 10K+ line changes (I would like to do that in the next PR, if possible). I had to change the syntax for view creation in `exists-having.sql` slightly, and this is reflected in the `analyzer-results` file, but crucially, the query output (in the `results` file) are not changed. Note that this will not be applicable to many of the current sql tests we have due to: - Incompatible SQL syntax between spark sql and postgres. - Incompatible data types. - Difference in numerical precision with doubles. - Missing functions in either system. - Test files with specific configs, such as ANSI, count bug etc. ### Why are the changes needed? For correctness checking of our SQLQueryTestSuites, we want to run SQLQueryTestSuites with Postgres as a reference DBMS. This can be easily extensible to other DBMS. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? This is a test-related PR, does not affect system behaviors. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#44084 from andylam-db/crossdbms. Authored-by: Andy Lam <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

andylam-db added 6 commits November 29, 2023 11:02

Initial commit

612396c

Add CrossDbmsQueryTestSuite

83e6a51

Small changes for niceness

c691f83

Save point

24a02f0

Add sqllogictest-select1.sql in postgres-crosstest input file

315847a

Add header for class suite

5e087e1

github-actions bot added the SQL label Nov 29, 2023

andylam-db added 5 commits November 29, 2023 14:09

Small changes

33b52b2

Trying to get the input files set up..

81202c4

Passing test!!

c9fba5b

Ignore all earlier tests first

8b5ed22

Small comment changes

fea3975

andylam-db marked this pull request as ready for review November 30, 2023 00:46

andylam-db changed the title ~~Add CrossDbmsQueryTestSuites, which allows generating golden files with Postgres/other DBMS~~ [SPARK-46179] Add CrossDbmsQueryTestSuites, which allows generating golden files with Postgres/other DBMS Nov 30, 2023

More comments

678a0ef

andylam-db changed the title ~~[SPARK-46179] Add CrossDbmsQueryTestSuites, which allows generating golden files with Postgres/other DBMS~~ [SQL][SPARK-46179] Add CrossDbmsQueryTestSuites, which allows generating golden files with Postgres/other DBMS Nov 30, 2023

HyukjinKwon changed the title ~~[SQL][SPARK-46179] Add CrossDbmsQueryTestSuites, which allows generating golden files with Postgres/other DBMS~~ [SPARK-46179][SQL] Add CrossDbmsQueryTestSuites, which allows generating golden files with Postgres/other DBMS Nov 30, 2023

dtenedor reviewed Nov 30, 2023

View reviewed changes

andylam-db added 11 commits November 30, 2023 10:27

Address simple comments

309a7e2

A little more small changes..

5c52b9c

More small changes and removal of redundant code

ad867df

Generate golden files for SQLQueryTestSuite

bef8011

Revert "Generate golden files for SQLQueryTestSuite"

a6e7e2a

This reverts commit bef8011.

Ignore "postgres-crosstest" in SQLQueryTestSuite

1409d63

Add comment to clarify why it is ignored

a552d86

Fix compilation failures

7a9cb80

Regenerate golden files for sqllogictest-select1.sql.out

66f41a5

Tiny changes..

69614d3

Add postgresql back to ignorelist

2bf43df

andylam-db commented Nov 30, 2023

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/crossdbms/CrossDbmsQueryTestSuite.scala Outdated Show resolved Hide resolved

agubichev approved these changes Jan 3, 2024

View reviewed changes

Fix silly paste

5d4c577

andylam-db added 2 commits January 2, 2024 23:15

Fix unintended removal of results file

68ea6f2

In middle of refactoring..

1e1a283

andylam-db force-pushed the crossdbms branch from 12e4dbc to b1d87e6 Compare January 3, 2024 20:54

andylam-db added 4 commits January 3, 2024 14:25

Major refactoring in progress..

5756c8e

Passing tests

81a61f7

Add documnetation and delete previous files

cf7b044

Move functions from PostgreSQLQueryTestSuite -> CrossDbmsQueryTestSuite

a32a693

andylam-db force-pushed the crossdbms branch from 002433c to a32a693 Compare January 3, 2024 23:15

github-actions bot removed the DOCS label Jan 3, 2024

Merge master..

e08f7a4

andylam-db requested a review from cloud-fan January 4, 2024 00:12

cloud-fan reviewed Jan 4, 2024

View reviewed changes

Merge branch 'master' into crossdbms

18f821f

cloud-fan approved these changes Jan 4, 2024

View reviewed changes

cloud-fan closed this in f9ca519 Jan 5, 2024

andylam-db changed the title ~~[SPARK-46179][SQL] Add CrossDbmsQueryTestSuites, which runs other DBMS against golden files with other DBMS, starting with Postgres~~ [SPARK-46179][SQL] Add CrossDbmsQueryTestSuites, which runs other DBMS against golden files, starting with Postgres Jan 24, 2024

[SPARK-46179][SQL] Add CrossDbmsQueryTestSuites, which runs other DBMS against golden files, starting with Postgres #44084

[SPARK-46179][SQL] Add CrossDbmsQueryTestSuites, which runs other DBMS against golden files, starting with Postgres #44084

Uh oh!

Conversation

andylam-db commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

dtenedor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andylam-db commented Jan 3, 2024

Uh oh!

cloud-fan commented Jan 3, 2024

Uh oh!

andylam-db commented Jan 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan Jan 4, 2024

Choose a reason for hiding this comment

Uh oh!

andylam-db Jan 4, 2024

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jan 4, 2024

Uh oh!

andylam-db commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andylam-db commented Jan 4, 2024

Uh oh!

cloud-fan commented Jan 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

andylam-db commented Nov 29, 2023 •

edited

Loading

andylam-db commented Jan 3, 2024 •

edited

Loading

andylam-db commented Jan 4, 2024 •

edited

Loading