alamb commented on code in PR #7284:
URL: https://github.com/apache/arrow-datafusion/pull/7284#discussion_r1294436100
##########
.github/workflows/dev_pr/labeler.yml:
##########
@@ -47,4 +47,4 @@ substrait:
- datafusion/substrait/**/*
sqllogictest:
- - datafusion/core/tests/sqllogictests/**/*
Review Comment:
This illustrates that the sqllogictest code is no longer so deeply nested in
the tree, which I think is a good change overall
##########
datafusion/sqllogictest/Cargo.toml:
##########
@@ -44,14 +44,25 @@ rust_decimal = {version = "1.27.0"}
log = "^0.4"
sqllogictest = "0.15.0"
sqlparser.workspace = true
+tempfile = "3"
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"}
Review Comment:
datafusion depends on `futures` anyways so this is not a new dependency
##########
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"] }
Review Comment:
Removing this line to remove the circular dependency is the entire reason
for this PR
##########
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 {
Review Comment:
I moved the logic for creating TestContext into the
`datafusion_sqllogictest` library (and out of the runner) to avoid making the
runner even longer
##########
.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
Review Comment:
Now running the posgres compatibility tests requires a feature flag, which I
think is a good thing and will speed up normal developer workflows
##########
.github/workflows/rust.yml:
##########
@@ -229,17 +229,17 @@ jobs:
rust-version: stable
- name: Generate benchmark data and expected query results
run: |
- mkdir -p datafusion/core/tests/sqllogictests/test_files/tpch/data
+ mkdir -p datafusion/sqllogictest/test_files/tpch/data
git clone https://github.com/databricks/tpch-dbgen.git
cd tpch-dbgen
make
./dbgen -f -s 0.1
- mv *.tbl ../datafusion/core/tests/sqllogictests/test_files/tpch/data
+ mv *.tbl ../datafusion/sqllogictest/test_files/tpch/data
- name: Verify that benchmark queries return expected results
run: |
- export TPCH_DATA=`realpath
datafusion/core/tests/sqllogictests/test_files/tpch/data`
+ export TPCH_DATA=`realpath
datafusion/sqllogictest/test_files/tpch/data`
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
Review Comment:
Arguable it is easier to run these tests now
##########
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
Review Comment:
moved to test_context.rs
##########
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';
Review Comment:
Eventually it might make sense to move the data files, but for now I just
updated the test paths
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
