Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sqllogictest] Run tests on Windows #5870

Merged
merged 1 commit into from
Apr 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ use self::error::{DFSqlLogicTestError, Result};
use async_trait::async_trait;
use datafusion::arrow::record_batch::RecordBatch;
use datafusion::prelude::SessionContext;
use datafusion_sql::parser::DFParser;
use sqllogictest::DBOutput;

mod error;
Expand Down Expand Up @@ -64,7 +63,7 @@ impl sqllogictest::AsyncDB for DataFusion {

/// [`Runner`] calls this function to perform sleep.
///
/// The default implementation is `std::thread::sleep`, which is universial to any async runtime
/// The default implementation is `std::thread::sleep`, which is universal to any async runtime
/// but would block the current thread. If you are running in tokio runtime, you should override
/// this by `tokio::time::sleep`.
async fn sleep(dur: Duration) {
Expand All @@ -73,12 +72,7 @@ impl sqllogictest::AsyncDB for DataFusion {
}

async fn run_query(ctx: &SessionContext, sql: impl Into<String>) -> Result<DFOutput> {
let sql = sql.into();
// check if the sql is more than one statement
if let Ok(mut statements) = DFParser::parse_sql(&sql) {
statements.pop_front().expect("at least one SQL statement");
}
Comment on lines -78 to -80
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This piece was needed to have workarounds for creating tables without values. It is not needed anymore.

let df = ctx.sql(sql.as_str()).await?;
let df = ctx.sql(sql.into().as_str()).await?;

let types = normalize::convert_schema_to_types(df.schema().fields());
let results: Vec<RecordBatch> = df.collect().await?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,18 @@ fn workspace_root() -> object_store::path::Path {
.expect("parent of datafusion")
.to_string_lossy();

object_store::path::Path::parse(workspace_root).unwrap()
let sanitized_workplace_root = if cfg!(windows) {
// Object store paths are delimited with `/`, e.g. `D:/a/arrow-datafusion/arrow-datafusion/testing/data/csv/aggregate_test_100.csv`.
// The default windows delimiter is `\`, so the workplace path is `D:\a\arrow-datafusion\arrow-datafusion`.
workspace_root.replace(std::path::MAIN_SEPARATOR, object_store::path::DELIMITER)
} else {
workspace_root.to_string()
};

object_store::path::Path::parse(sanitized_workplace_root).unwrap()
}

// holds the root directory (
// holds the root directory
lazy_static! {
static ref WORKSPACE_ROOT: object_store::path::Path = workspace_root();
}
Expand Down
25 changes: 21 additions & 4 deletions datafusion/core/tests/sqllogictests/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

use std::error::Error;
use std::path::{Path, PathBuf};
#[cfg(target_family = "windows")]
use std::thread;

use log::info;
use sqllogictest::strict_column_validator;
Expand All @@ -33,16 +35,31 @@ mod utils;
const TEST_DIRECTORY: &str = "tests/sqllogictests/test_files/";
const PG_COMPAT_FILE_PREFIX: &str = "pg_compat_";

#[tokio::main]
#[cfg(target_family = "windows")]
pub async fn main() -> Result<(), Box<dyn Error>> {
println!("Skipping test on windows");
Ok(())
pub fn main() {
// Tests from `tpch.slt` fail with stackoverflow with the default stack size.
thread::Builder::new()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tpch.slt test fails with stack overflow errors. The stack size on Windows seems to be 1Mb by default vs 2MB on Linux. I tried debugging what causes the error, but it seems that the stack is just deep enough due to the recursive design of the physical planner.

In case somebody with more understanding takes a closer look, I added a stack trace for the overflow as well as a test to reproduce the issue with unit tests on any platform here https://gist.github.com/melgenek/b6acf4467ee10c1941f3b103a17e1b1e.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have hit this before (e.g. #4065) -- I think cranking up the default size is fine.

Specifically, release builds seem to use much less stack so this is primarily a test issue rather than a production issue

.stack_size(2 * 1024 * 1024) // 2 MB
.spawn(move || {
tokio::runtime::Builder::new_current_thread()
.enable_all()
.build()
.unwrap()
.block_on(async { run_tests().await })
.unwrap()
})
.unwrap()
.join()
.unwrap();
}

#[tokio::main]
#[cfg(not(target_family = "windows"))]
pub async fn main() -> Result<(), Box<dyn Error>> {
run_tests().await
}

async fn run_tests() -> Result<(), Box<dyn Error>> {
// Enable logging (e.g. set RUST_LOG=debug to see debug logs)
env_logger::init();

Expand Down