Move sqllogictests to sqllogictests crate to break cyclic dependency #7284

alamb · 2023-08-14T22:21:05Z

Which issue does this PR close?

Closes #7281

Rationale for this change

feat: add sqllogictests crate #7134 introduced a circular dependency which means we can't publish to crates.io
Elevating the sqllogictests into a less deep directory hierarchy will make it more discoverable

What changes are included in this PR?

Move sqllogictest drivers from datafusion/core/tests/sqlloictest to datafusion/sqllogictest
Update TestContext and include it in the datafusion_sqllogictest module
Update CI jobs that use sqllogictest to use the new module
Update readme
Leave backwards compatibility MOVED.md

Are these changes tested?

Yes, both manually and via CI

Are there any user-facing changes?

No, but this likely will affect developer tests

New Command

sqllogictest, is now run like this:

cargo test --test sqllogictest

The old command was like this

cargo test -p datafusion --test sqllogictest

Bonus it also fixes the error called out by @Dandandan in the mailing list thread: https://lists.apache.org/thread/mbm8smv4rombbbsnfgll922768b103z6

   Compiling datafusion-sqllogictest v29.0.0 (/Users/alamb/Software/arrow-datafusion2/datafusion/sqllogictest)
warning: function `decimal_to_str` is never used
  --> datafusion/sqllogictest/src/engines/conversion.rs:85:15
   |
85 | pub(crate) fn decimal_to_str(value: Decimal) -> String {
   |               ^^^^^^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

alamb · 2023-08-15T10:36:06Z

.github/workflows/dev_pr/labeler.yml

@@ -47,4 +47,4 @@ substrait:
  - datafusion/substrait/**/*

 sqllogictest:
-  - datafusion/core/tests/sqllogictests/**/*


This illustrates that the sqllogictest code is no longer so deeply nested in the tree, which I think is a good change overall

alamb · 2023-08-15T10:36:50Z

.github/workflows/rust.yml

@@ -270,7 +270,7 @@ jobs:
          rustup toolchain install stable
          rustup default stable
      - name: Run sqllogictest
-        run: PG_COMPAT=true PG_URI="postgresql://postgres:postgres@localhost:$POSTGRES_PORT/db_test" cargo test -p datafusion --test sqllogictests
+        run: PG_COMPAT=true PG_URI="postgresql://postgres:postgres@localhost:$POSTGRES_PORT/db_test" cargo test --features=postgres --test sqllogictests


Now running the posgres compatibility tests requires a feature flag, which I think is a good thing and will speed up normal developer workflows

alamb · 2023-08-15T10:37:01Z

.github/workflows/rust.yml

          cargo test serde_q --profile release-nonlto --features=ci -- --test-threads=1
-          INCLUDE_TPCH=true cargo test -p datafusion --test sqllogictests
+          INCLUDE_TPCH=true cargo test --test sqllogictests


Arguable it is easier to run these tests now

alamb · 2023-08-15T10:37:27Z

datafusion/core/Cargo.toml

@@ -102,7 +102,6 @@ bigdecimal = "0.4.1"
 criterion = { version = "0.5", features = ["async_tokio"] }
 csv = "1.1.6"
 ctor = "0.2.0"
-datafusion-sqllogictest = { path = "../sqllogictest", version = "29.0.0", features = ["postgres"] }


Removing this line to remove the circular dependency is the entire reason for this PR

alamb · 2023-08-15T10:38:31Z

datafusion/sqllogictest/Cargo.toml

 thiserror = "1.0.44"
 tokio = {version = "1.0"}
 bytes = {version = "1.4.0", optional = true}
-futures = {version = "0.3.28", optional = true}
+futures = {version = "0.3.28"}


datafusion depends on futures anyways so this is not a new dependency

alamb · 2023-08-15T10:39:36Z

datafusion/sqllogictest/bin/sqllogictests.rs

@@ -121,7 +117,7 @@ async fn run_test_file(test_file: TestFile) -> Result<()> {
        relative_path,
    } = test_file;
    info!("Running with DataFusion runner: {}", path.display());
-    let Some(test_ctx) = context_for_test_file(&relative_path).await else {
+    let Some(test_ctx) = TestContext::try_new_for_test_file(&relative_path).await else {


I moved the logic for creating TestContext into the datafusion_sqllogictest library (and out of the runner) to avoid making the runner even longer

alamb · 2023-08-15T10:39:49Z

datafusion/sqllogictest/bin/sqllogictests.rs

@@ -250,96 +254,6 @@ fn read_dir_recursive<P: AsRef<Path>>(path: P) -> Box<dyn Iterator<Item = PathBu
    )
 }

-/// Create a SessionContext, configured for the specific test, if


moved to test_context.rs

alamb · 2023-08-15T10:40:43Z

datafusion/sqllogictest/test_files/array.slt

@@ -2289,7 +2289,7 @@ select make_array(1.0, '2', null)
 ### FixedSizeListArray

 statement ok
-CREATE EXTERNAL TABLE fixed_size_list_array STORED AS PARQUET LOCATION 'tests/data/fixed_size_list_array.parquet';
+CREATE EXTERNAL TABLE fixed_size_list_array STORED AS PARQUET LOCATION '../core/tests/data/fixed_size_list_array.parquet';


Eventually it might make sense to move the data files, but for now I just updated the test paths

alamb · 2023-08-15T11:36:08Z

cc @melgenek

melgenek

I only briefly skimmed through the change. Removing the cyclic dependency looks sane to me.

alamb · 2023-08-15T14:09:18Z

Thank you @melgenek

alamb added 2 commits August 14, 2023 17:56

Move sqllogictests to datafusion-sqllogictest module

7ca1df0

Port code to be in datafusion

c83cbdd

github-actions bot added the core Core DataFusion crate label Aug 14, 2023

alamb mentioned this pull request Aug 14, 2023

Circular dependency between datafusion and datafusion-sqllogictest crates prevents 0.29.0 from being published to crates.io #7281

Closed

alamb added 8 commits August 15, 2023 06:16

Update expected output

6ee65b0

Merge remote-tracking branch 'apache/main' into alamb/move_sqllogic_test

2b579b9

update workflows

d812720

update flow

a0a04d7

update postgres check

8d179d2

fmt

1862f80

clippy

d1a0732

Add moved

0c0d848

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Aug 15, 2023

Add RAT to MOVED.md

8525899

alamb commented Aug 15, 2023

View reviewed changes

alamb requested a review from andygrove August 15, 2023 11:36

alamb marked this pull request as ready for review August 15, 2023 11:36

alamb changed the title ~~Move sqllogictests to sqllogictests crate~~ Move sqllogictests to sqllogictests crate to break cyclic dependency Aug 15, 2023

Fix tpch instructions and tests

a33b4da

alamb mentioned this pull request Aug 15, 2023

Implement Support for Copy To Logical and Physical plans #7283

Merged

melgenek approved these changes Aug 15, 2023

View reviewed changes

yjshen approved these changes Aug 15, 2023

View reviewed changes

yjshen merged commit 90484bb into apache:main Aug 15, 2023

alamb deleted the alamb/move_sqllogic_test branch August 15, 2023 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move sqllogictests to sqllogictests crate to break cyclic dependency #7284

Move sqllogictests to sqllogictests crate to break cyclic dependency #7284

alamb commented Aug 14, 2023 •

edited

Loading

alamb Aug 15, 2023

alamb Aug 15, 2023

alamb Aug 15, 2023

alamb Aug 15, 2023

alamb Aug 15, 2023

alamb Aug 15, 2023

alamb Aug 15, 2023

alamb Aug 15, 2023

alamb commented Aug 15, 2023

melgenek left a comment

alamb commented Aug 15, 2023

Move sqllogictests to sqllogictests crate to break cyclic dependency #7284

Move sqllogictests to sqllogictests crate to break cyclic dependency #7284

Conversation

alamb commented Aug 14, 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?

New Command

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

alamb commented Aug 15, 2023

melgenek left a comment

Choose a reason for hiding this comment

alamb commented Aug 15, 2023

alamb commented Aug 14, 2023 •

edited

Loading