alamb commented on code in PR #13936:
URL: https://github.com/apache/datafusion/pull/13936#discussion_r1899135647
##########
datafusion/sqllogictest/README.md:
##########
@@ -160,7 +161,7 @@ cargo test --test sqllogictests -- information
Test files that start with prefix `pg_compat_` verify compatibility
with Postgres by running the same script files both with DataFusion and with
Postgres
-In order to run the sqllogictests running against a previously running
Postgres instance, do:
+In order to have the sqllogictest run against an existing running Postgres
instance, do:
Review Comment:
Thank you for cleaning up the word salad
##########
datafusion/sqllogictest/src/engines/datafusion_engine/normalize.rs:
##########
@@ -239,6 +239,10 @@ pub fn cell_to_string(col: &ArrayRef, row: usize) ->
Result<String> {
let key = dict.normalized_keys()[row];
Ok(cell_to_string(dict.values(), key)?)
}
+ // only added because of a bug in v 1.0.4 (is) of
lexical-write-integer
Review Comment:
Is this the same as
- https://github.com/Alexhuszagh/rust-lexical/issues/191
If so I think we could remove the workaround now as it has been subsequently
fixed
##########
datafusion/sqllogictest/README.md:
##########
@@ -205,6 +215,34 @@ Then you need to add `INCLUDE_TPCH=true` to run tpch tests:
INCLUDE_TPCH=true cargo test --test sqllogictests
```
+## Running Tests: `sqlite`
+
+Test files in `data/sqlite` directory of the datafusion-testing crate were
+sourced from the sqlite test suite and have been cleansed and updated to
Review Comment:
Could you also please add a link to what "sqlite test suite" means?
I think it means https://www.sqlite.org/sqllogictest/dir?ci=tip
##########
datafusion/sqllogictest/bin/sqllogictests.rs:
##########
@@ -16,57 +16,129 @@
// under the License.
use clap::Parser;
+use datafusion_common::instant::Instant;
use datafusion_common::utils::get_available_parallelism;
+use datafusion_common::{exec_datafusion_err, exec_err, DataFusionError,
Result};
+use datafusion_common_runtime::SpawnedTask;
use datafusion_sqllogictest::{DataFusion, TestContext};
use futures::stream::StreamExt;
+use indicatif::{
+ HumanDuration, MultiProgress, ProgressBar, ProgressDrawTarget,
ProgressStyle,
+};
use itertools::Itertools;
-use log::info;
-use sqllogictest::{strict_column_validator, Normalizer};
+use log::Level::{Info, Warn};
+use log::{info, log_enabled, warn};
+#[cfg(feature = "postgres")]
+use once_cell::sync::Lazy;
+use sqllogictest::{
+ parse_file, strict_column_validator, AsyncDB, Condition, Normalizer,
Record,
+ Validator,
+};
+#[cfg(feature = "postgres")]
+use std::env::set_var;
use std::ffi::OsStr;
use std::fs;
+#[cfg(feature = "postgres")]
Review Comment:
Perhaps as a follow on PR the code that manages the postgres container could
be moved into its own module (like `postgres_container.rs` or something so we
only needed one `#[cfg(feature = "postgres")]`
I suspect this would also make the code a bit easier to reason about
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
