[sqllogictets] Remove postgres container orchestration #5015

alamb · 2023-01-21T14:32:27Z

Which issue does this PR close?

Rationale for this change

#4834 added a way to run sqllogictests using postgres 🦾 (kudos to @melgenek )

However, it currently orchestrates the postgres containers with rust test code which means running these tests requires docker to run the (full) datafusion test suit locally

What changes are included in this PR?

Remove container orchestration logic
Implement special support for COPY so it can be run from the postgres tests
Postgres connection configuration is specified by PG_DSN="postgresql://postgres@127.0.0.1/postgres" environment
Add documentation on how to run with existing postgres container

PG_COMPAT=true PG_DSN="postgresql://postgres@127.0.0.1/postgres" cargo test -p datafusion --test sqllogictests

Are these changes tested?

Yes, in CI

Are there any user-facing changes?

Not really -- only to other devs

… postgres.rs

alamb · 2023-01-21T14:33:31Z

.github/workflows/rust.yml

@@ -232,9 +232,21 @@ jobs:
    name: "Run sqllogictest with Postgres runner"
    needs: [linux-build-lib]
    runs-on: ubuntu-latest
+    services:
+      postgres:


this starts the container using the code from integration-tests (above in this file) rather than running docker from within the harness

alamb · 2023-01-21T14:35:54Z

datafusion/core/tests/sqllogictests/postgres/postgres_create_table.sql

@@ -1,21 +0,0 @@
-CREATE TABLE aggregate_test_100_by_sql


these statements are now run in the individual setup files

alamb · 2023-01-21T14:36:47Z

datafusion/core/tests/sqllogictests/src/engines/postgres/mod.rs

+    /// ```
+    ///
+    /// And read the file locally.
+    async fn run_copy_command(&mut self, sql: &str) -> Result<DBOutput> {


I was quite pleased with this -- it allows the tests to run COPY FROM 'file'

alamb · 2023-01-21T14:37:25Z

datafusion/core/tests/sqllogictests/test_files/aggregate.slt

@@ -15,6 +15,28 @@
 # specific language governing permissions and limitations
 # under the License.

+
+statement ok


drive by clean up -- this can be run directly in the .slt file

alamb · 2023-01-21T14:37:46Z

datafusion/core/tests/sqllogictests/test_files/pg_compat_simple.slt

+## Setup test for postgres
+###
+
+onlyif postgres


I used the onlyif statement to run different setups for DataFusion and posgres

alamb · 2023-01-21T18:58:48Z

cc @melgenek

melgenek

I like the approach of having setup per file. It is more clear where data comes from this way.

Removing a container-per-file approach ditches isolation for tests. I would suggest having a schema per test file.

melgenek · 2023-01-21T19:42:44Z

datafusion/core/tests/sqllogictests/src/engines/postgres/mod.rs

+        debug!("Using posgres dsn: {dsn}");
+
+        let config = tokio_postgres::Config::from_str(&dsn)?;
+
        let mut retry = 0;


You can probably get rid of the retry. It was sort of a hack because queries were executed immediately after a docker startup. And sometimes Postgres wasn't fully ready. Assuming that Postgres gets started in advance, you can assume that the connection succeeds.

I think some of the options from the container setup (that I copied from @jimexist ) may help.

ports: - 5432/tcp options: >- --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5

removed retry in c41a0a8

datafusion/core/tests/sqllogictests/src/engines/postgres/mod.rs

datafusion/core/tests/sqllogictests/README.md

melgenek · 2023-01-21T20:44:16Z

datafusion/core/tests/sqllogictests/README.md

+`ORDER BY` will not match DataFusion and the tests will diff.
+
+```shell
+docker run \


Suggested change

docker run \

docker run --rm \

I think --rm removes the container. I normally prefer to leave them around for debugging

melgenek · 2023-01-21T20:59:57Z

datafusion/core/tests/sqllogictests/test_files/pg_compat_simple.slt

+
+onlyif postgres
+statement ok
+DROP TABLE IF EXISTS aggregate_test_100_by_sql;


Dropping a table seems like a hack to the fact that Postgres database is reused for multiple test files.
It would be nice to introduce isolation by, for example, having a schema per file. The schema could be either random or based on the test file name.

I drafted a very basic implementation of the approach. melgenek@3394c87

Thanks -- this is a good point @melgenek

At the very least the tests should clean up after themselves (as in I should put a drop_table at the end). I like the idea of a separate schema, however. I will try and add that shortly

I was thinking more about the relevance of schemas and isolation.

If Datafusion's test suite ever becomes big enough, you would be able to parallelize it. And schemas would help run multiple files in parallel without interference.

Co-authored-by: Yevhenii Melnyk <melnyk.yevhenii@gmail.com>

…gres

xudong963

Thanks andrew

ursabot · 2023-01-23T13:31:59Z

Benchmark runs are scheduled for baseline = 25ab1f9 and contender = 930c8de. 930c8de is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Remove posgres container orchestration and move connection logic into…

c9c1179

… postgres.rs

github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jan 21, 2023

alamb commented Jan 21, 2023

View reviewed changes

prettier

e86339c

alamb changed the title ~~[sqllogictets] Remove postgres container~~ [sqllogictets] Remove postgres container orchestration Jan 21, 2023

fix up ci

35315b4

alamb marked this pull request as ready for review January 21, 2023 18:58

alamb requested a review from xudong963 January 21, 2023 18:58

melgenek approved these changes Jan 21, 2023

View reviewed changes

alamb mentioned this pull request Jan 22, 2023

[sqllogictest] Don't orchestrate the postgres containers with rust / docker #5009

Closed

alamb and others added 7 commits January 22, 2023 07:03

Apply suggestions from code review

5569bee

Co-authored-by: Yevhenii Melnyk <melnyk.yevhenii@gmail.com>

Remove retries

c41a0a8

rename PG_DSN --> PG_URI

99a2efd

Schema per sqllogictest file

649727b

clean up

437df59

clippy

2662509

Merge remote-tracking branch 'apache/master' into alamb/external_post…

dc81ec3

…gres

melgenek approved these changes Jan 23, 2023

View reviewed changes

alamb added 2 commits January 23, 2023 06:38

remove overly aggressive cleanup

797f605

Merge remote-tracking branch 'apache/master' into alamb/external_post…

1e33041

…gres

xudong963 approved these changes Jan 23, 2023

View reviewed changes

xudong963 merged commit 930c8de into apache:master Jan 23, 2023

alamb deleted the alamb/external_postgres branch July 26, 2024 10:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sqllogictets] Remove postgres container orchestration #5015

[sqllogictets] Remove postgres container orchestration #5015

alamb commented Jan 21, 2023 •

edited

Loading

alamb Jan 21, 2023

alamb Jan 21, 2023

alamb Jan 21, 2023

alamb Jan 21, 2023

alamb Jan 21, 2023

alamb commented Jan 21, 2023

melgenek left a comment

melgenek Jan 21, 2023

alamb Jan 22, 2023

alamb Jan 22, 2023

melgenek Jan 21, 2023

alamb Jan 22, 2023

melgenek Jan 21, 2023

alamb Jan 22, 2023

melgenek Jan 22, 2023

xudong963 left a comment

ursabot commented Jan 23, 2023

[sqllogictets] Remove postgres container orchestration #5015

[sqllogictets] Remove postgres container orchestration #5015

Conversation

alamb commented Jan 21, 2023 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jan 21, 2023

melgenek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xudong963 left a comment

Choose a reason for hiding this comment

ursabot commented Jan 23, 2023

alamb commented Jan 21, 2023 •

edited

Loading