(#4462) Postgres compatibility tests using sqllogictest #4834

melgenek · 2023-01-06T20:55:27Z

Which issue does this PR close?

Closes #4462.

Rationale for this change

It is an example implementation of the sqllogictest tests that run on both Postgres and Datafusion and compare results.

What changes are included in this PR?

introduce an implementation of a Postgres runner for sqllogictest
rewrite integration-tests
to sqllogictestformat
run a subset of tests with both Datafusion and Postgres to verify that both produce the same results.

Are these changes tested?

There is a dedicated Github action that runs Postgres sqllogictest tests.

Are there any user-facing changes?

N/A

datafusion/core/Cargo.toml

datafusion/core/tests/sqllogictests/postgres/test_files/simple_aggregation.slt

...n/core/tests/sqllogictests/postgres/test_files/simple_window_partition_order_aggregation.slt

datafusion/core/tests/sqllogictests/postgres/test_files/values_list.slt

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

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

melgenek · 2023-01-06T21:16:24Z

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

+ )
+ }
+
+ pub async fn connect_with_retry(


In my implementation a docker container starts before each test. And sometimes first connections fail. I didn't find a solution to deterministically define when a container is fully ready, so I introduced a connection retry.

👍

I think in general the approach of orchestrating the docker containers using github CI worked well rather than restarting the containers within the tests

datafusion/core/tests/sqllogictests/src/main.rs

melgenek · 2023-01-06T21:22:05Z

datafusion/core/tests/sqllogictests/src/setup.rs

@@ -111,23 +111,23 @@ fn register_median_test_tables(ctx: &SessionContext) {
 }
 }

-async fn register_aggregate_csv_by_sql(ctx: &SessionContext) {
+pub async fn register_aggregate_csv_by_sql(ctx: &SessionContext) {


I re-used the existing table creation function and updated the data types according to the sql from integration-tests https://github.com/apache/arrow-datafusion/blob/master/integration-tests/create_test_table.sql.

I can, of course, introduce another function.

integration-tests/create_test_table_postgres.sql

alamb

Thank you @melgenek -- this is great progress

datafusion/core/Cargo.toml

datafusion/core/tests/sqllogictests/postgres/test_files/simple_aggregation.slt

datafusion/core/tests/sqllogictests/postgres/test_files/self_join_with_alias.slt

datafusion/core/tests/sqllogictests/postgres/test_files/simple_except.slt

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

datafusion/core/tests/sqllogictests/src/main.rs

Merge slt files

melgenek · 2023-01-18T21:34:04Z

datafusion/core/Cargo.toml

+half = "2.2.1"
 parquet-test-utils = { path = "../../parquet-test-utils" }
+postgres-types = { version = "0.2.4", features = ["derive", "with-chrono-0_4"] }
 rstest = "0.16.0"
+rust_decimal = { version = "1.27.0", features = ["tokio-pg"] }
 sqllogictest = "0.10.0"
 test-utils = { path = "../../test-utils" }
+testcontainers = "0.14.0"
 thiserror = "1.0.37"
-
+tokio-postgres = "0.7.7"


Here are the reasons for these dependencies:

half - f16 type for Datafusion

testcontainers - creates a fresh docker container with Postgres for each sqllogictest file.

postgres-types and tokio-postgres - these are required for writing a Postgres client

rust_decimal - converts Postgres "numeric" type to a rust type

bigdecimal - provides a common type to do floating number rounding. rust_decimal, unfortunately, doesn't handle numbers of arbitrary precision. For example, rust_decimal could not parse 26156334342021890000000000000000000000 that is currently present in one of .slt tests in Datafusion.

datafusion/core/Cargo.toml

melgenek · 2023-01-18T21:42:42Z

datafusion/core/tests/sqllogictests/src/engines/conversion.rs

+}
+
+pub fn big_decimal_to_str(value: BigDecimal) -> String {
+ value.round(12).normalized().to_string()


All numbers are rounded to 12 decimal digits. Without explicit types Postgres and Datafusion can choose different underlying types. For example, Postgres could choose to use numeric when Datafusion uses int. In order to compare results, all floating number types are converted to the same number of decimal points.

12 is chosen to pass the existing set of tests. I think it could produce errors, for example, when rounding f16 to 12 digits. I would probably use 3 (or 4) decimal digits if high precision is not required for Postgres compatibility tests. 3 or 4 is an expected number of digits for 16 bit binary according to IEEE_754, so it should be safer to round to the smallest possible data type.

melgenek · 2023-01-18T21:45:42Z

datafusion/core/tests/sqllogictests/src/engines/datafusion/normalize.rs

+ if !col.is_valid(row) {
+ // represent any null value with the string "NULL"
+ Ok(NULL_STR.to_string())
+ } else if is_pg_compatibility_test {


If a test is a Postgres compatibility test then some conversions apply. The same conversions are used in the Postgres client implementation.

This is needed, because existing sqllogictests use higher precision of floating number results, which would produce different results in Postgres.

👍 I am not sure the super high precision floating point results are necessary. I wonder if we could simply use the lower precision normalization in all the tests (both in pgcompat and non pg compat) 🤔

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

melgenek · 2023-01-18T21:54:24Z

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

+ }};
+}
+
+fn cell_to_string(row: &Row, column: &Column, idx: usize) -> String {


The implementation of Postgres client is based on the "postgres-extended" from the sqllogictest-rs repo https://github.com/risinglightdb/sqllogictest-rs/blob/2fb7e36e1857fd6b7949956b496c26ddc463f858/sqllogictest-bin/src/engines/postgres_extended.rs.

All the type conversions are now through rust like in the Datafusion. So every type has to be converted to a string explicitly.

I implemented the types marked as implemented from https://github.com/apache/arrow-datafusion/blob/master/docs/source/user-guide/sql/data_types.md and made their text representation compatible with Datafusion.
I haven't implemented arrays.

melgenek · 2023-01-18T21:57:12Z

datafusion/core/tests/sqllogictests/src/main.rs

+ } else if options.postgres_runner {
+ if is_pg_compatibility_test {
+ run_test_file_with_postgres(&path, file_name).await?;
+ } else {
+ debug!("Skipping test file {:?}", path);
+ }


The tests which name starts with pg_compat_ would run with Datafusion during cargo test.
In order to check compatibility with Postgres, there is a github job that runs these tests with a Postgres runner.

Thus Datafusion runs all the time. And Postgres runs only explicitly. This way cargo test is not affected by the speed of running docker containers and executing Postgres queries.

melgenek · 2023-01-18T22:05:31Z

@alamb The pr is updated according to your previous review:

results are defined explicitly
Postgres runs only a subset of tests file prefixed with pg_compat_
Postgres runs only when the PG_COMPAT env var is set
Postgres client is fully rewritten to explicitly convert results according to their types. Datafusion client is updated to have the same conversions. This way text representations are comparable.

alamb · 2023-01-19T22:07:57Z

Thanks @melgenek -- this PR is on my list to review -- but I ran out of time today. I will plan to do so tomorrow

alamb

I really like this @melgenek -- Thank you, I have several changes I would like to make, but I also think we could make them as (fast) follow on PRs. Specifically:

Don't orchestrate the postgres containers with rust test code, rather orchestrate outside (either with an existing container or github CI, and document what is needed). Making this change will avoid requiring docker to run the datafusion tests,
Use the same normalization in postgres and non-postgres code (maybe by decreasing fidelity of datafusion test)
Remove existing postgres python integration tests

Given how large this PR is already I would be inclined to merge it as is and iterate as a follow on. What do you think @xudong963 @jimexist @avantgardnerio ?

alamb · 2023-01-20T14:26:38Z

datafusion/core/tests/sqllogictests/src/engines/datafusion/normalize.rs

+ if !col.is_valid(row) {
+ // represent any null value with the string "NULL"
+ Ok(NULL_STR.to_string())
+ } else if is_pg_compatibility_test {


👍 I am not sure the super high precision floating point results are necessary. I wonder if we could simply use the lower precision normalization in all the tests (both in pgcompat and non pg compat) 🤔

alamb · 2023-01-20T14:28:28Z

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

+ )
+ }
+
+ pub async fn connect_with_retry(


👍

I think in general the approach of orchestrating the docker containers using github CI worked well rather than restarting the containers within the tests

alamb · 2023-01-20T14:28:59Z

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

+ }};
+}
+
+fn cell_to_string(row: &Row, column: &Column, idx: usize) -> String {


xudong963

Oh, this is a huge step! Thanks @melgenek

I agree with @alamb , we can iterate it at next PRs.

avantgardnerio · 2023-01-20T16:06:05Z

Don't orchestrate the postgres containers with rust test code

Good catch... I 💯 % agree with this.

alamb · 2023-01-20T18:16:57Z

🤔 it seems like risinglightdb/sqllogictest-rs#154 may be related to this PR (they have factored out their own postgres driver)

I think I can make time to help this PR along this weekend if that would assist.

melgenek · 2023-01-20T19:59:24Z

I have several changes I would like to make, but I also think we could make them as (fast) follow on PRs. Specifically:

Don't orchestrate the postgres containers with rust test code, rather orchestrate outside (either with an existing container or github CI, and document what is needed). Making this change will avoid requiring docker to run the datafusion tests,

Use the same normalization in postgres and non-postgres code (maybe by decreasing fidelity of datafusion test)

Remove existing postgres python integration tests

This sounds reasonable to me. I'll try to make prs for points 1 and 2 in the next few days.
For now, I merge master into this branch, so that it is mergeable.

🤔 it seems like risinglightdb/sqllogictest-rs#154 may be related to this PR (they have factored out their own postgres driver)

I saw this change, but I don't think that extraction is currently sufficient to fully replace implementation in this pr. The biggest part of the Postgres client is the results conversion. And if it is made pluggable into the client from sqllogictest-rs, then it would be almost like writing the whole client from scratch.
It seems to me, that a good idea would be to make primitive type conversions pluggable, so that Datafusion can apply the same floating point approximations, etc. And sqllogictest-rs would use the primitive type conversions for composite types, like arrays.

alamb · 2023-01-20T22:14:55Z

For now, I merge master into this branch, so that it is mergeable.

Thanks @melgenek . I filed issues for the follow on tasks:

alamb · 2023-01-20T22:16:37Z

Assuming this PR passes CI checks I plan to merge

xudong963 · 2023-01-21T02:02:28Z

merged, let's iterate!

ursabot · 2023-01-21T02:06:20Z

Benchmark runs are scheduled for baseline = b71cae8 and contender = 1d69f28. 1d69f28 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

alamb · 2023-01-21T11:05:42Z

Thanks again @melgenek and @xudong963 -- great to see this pushed along

melgenek added 3 commits January 3, 2023 20:50

Move Datafusion engine to a package

236bdc8

Run postgres and datafusion tests and compare outputs

d00a1de

clippy and fmt

8150314

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

melgenek mentioned this pull request Jan 6, 2023

Replace python based integration test with sqllogictest #4462

Closed

Remove unused line

d5369d0

melgenek commented Jan 6, 2023

View reviewed changes

alamb reviewed Jan 7, 2023

View reviewed changes

Floating types rounding

d06ef0f

xxchan mentioned this pull request Jan 17, 2023

consider exposing pre-built engines public risinglightdb/sqllogictest-rs#153

Closed

melgenek added 11 commits January 18, 2023 09:05

Make basic types compatible

f8c122b

Format with BigDecimal to keep arbitrary precision.

ff1e089

Fill postgres queries with results

55a49af

Run Postgres separately from DataFusion.

1a78bc6

Merge slt files

fmt

ad4acb5

clippy

fa7866a

Postgres step in github actions

70aa1c9

Run sqllogictest with Postgres on plain ubuntu

9bee2a3

Update sqllogictest readme

4de00da

Merge branch 'master' into 4462-complete-file-approach

de741df

Revert unnecessary change

44bb1de

melgenek commented Jan 18, 2023

View reviewed changes

melgenek added 2 commits January 18, 2023 22:58

Add license headers

b7b9e70

Don't log PG_COMPAT value

8453ed1

melgenek marked this pull request as ready for review January 18, 2023 22:05

melgenek requested a review from alamb January 18, 2023 22:05

melgenek added 2 commits January 19, 2023 18:34

clippy and fmt

5c2fc55

remove whitespaces in readme

4c7734d

alamb approved these changes Jan 20, 2023

View reviewed changes

xudong963 approved these changes Jan 20, 2023

View reviewed changes

Merge branch 'master' into 4462-complete-file-approach

81500c8

This was referenced Jan 20, 2023

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

Closed

[sqllogictest] Consolidate normalization code for the postgres and non-postgres paths #5010

Closed

[sqllogictest] Remove integration-tests directory #5011

Closed

xudong963 merged commit 1d69f28 into apache:master Jan 21, 2023

alamb mentioned this pull request Jan 21, 2023

[sqllogictets] Remove postgres container orchestration #5015

Merged

melgenek deleted the 4462-complete-file-approach branch January 25, 2023 11:34

andygrove mentioned this pull request Jan 27, 2023

pg_compat_window.slt fails when target_partitions >= 32 #5082

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(#4462) Postgres compatibility tests using sqllogictest #4834

(#4462) Postgres compatibility tests using sqllogictest #4834

melgenek commented Jan 6, 2023 •

edited

Loading

melgenek Jan 6, 2023

alamb Jan 20, 2023

melgenek Jan 6, 2023

alamb left a comment

melgenek Jan 18, 2023

melgenek Jan 18, 2023

melgenek Jan 18, 2023

alamb Jan 20, 2023

melgenek Jan 18, 2023

alamb Jan 20, 2023

melgenek Jan 18, 2023

melgenek commented Jan 18, 2023

alamb commented Jan 19, 2023

alamb left a comment

alamb Jan 20, 2023

alamb Jan 20, 2023

alamb Jan 20, 2023

xudong963 left a comment

avantgardnerio commented Jan 20, 2023

alamb commented Jan 20, 2023 •

edited

Loading

melgenek commented Jan 20, 2023

alamb commented Jan 20, 2023

alamb commented Jan 20, 2023

xudong963 commented Jan 21, 2023

ursabot commented Jan 21, 2023

alamb commented Jan 21, 2023

(#4462) Postgres compatibility tests using sqllogictest #4834

(#4462) Postgres compatibility tests using sqllogictest #4834

Conversation

melgenek commented Jan 6, 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

alamb 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

melgenek commented Jan 18, 2023

alamb commented Jan 19, 2023

alamb 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

xudong963 left a comment

Choose a reason for hiding this comment

avantgardnerio commented Jan 20, 2023

alamb commented Jan 20, 2023 • edited Loading

melgenek commented Jan 20, 2023

alamb commented Jan 20, 2023

alamb commented Jan 20, 2023

xudong963 commented Jan 21, 2023

ursabot commented Jan 21, 2023

alamb commented Jan 21, 2023

melgenek commented Jan 6, 2023 •

edited

Loading

alamb commented Jan 20, 2023 •

edited

Loading