Skip to content

Commit 02dae77

Browse files
authored
Speedup sqllogictests by running long running tests first (#20576)
## Which issue does this PR close? - part of #20524 - Follow on to #20566 from @kosiew ## Rationale for this change Our sqllogictests harness runs the queries in a single file serially, but runs multiple files in parallel Right now, the runtime of ```shell cargo test --profile=ci --test sqllogictests ``` Is domninated by a few long running tests -- so the sooner they are started, the sooner the overall suite finishes ## What changes are included in this PR? Bulding on #20566 from @kosiew adds a heuristic reordering of the tests when run so that the longest running are run first ## Are these changes tested? By CI and I ran performance tests manually ### on main On main this takes 8 seconds ```shell andrewlamb@Andrews-MacBook-Pro-3:~/Software/datafusion$ cargo test --profile=ci --test sqllogictests Finished `ci` profile [unoptimized] target(s) in 0.21s Running bin/sqllogictests.rs (target/ci/deps/sqllogictests-c4e4be8d5c9fd66e) Running with 16 test threads (available parallelism: 16) Completed 408 test files in 8 seconds ``` ## After test split After #20566 it takes 7 seconds to complete: ```shell andrewlamb@Andrews-MacBook-Pro-3:~/Software/datafusion$ cargo test --profile=ci --test sqllogictests Finished `ci` profile [unoptimized] target(s) in 0.20s Running bin/sqllogictests.rs (target/ci/deps/sqllogictests-c4e4be8d5c9fd66e) Running with 16 test threads (available parallelism: 16) Completed 411 test files in 7 seconds ``` ## This PR With this PR it takes 5 seconds: ```shell andrewlamb@Andrews-MacBook-Pro-3:~/Software/datafusion$ cargo test --profile=ci --test sqllogictests Compiling datafusion-sqllogictest v52.1.0 (/Users/andrewlamb/Software/datafusion/datafusion/sqllogictest) Finished `ci` profile [unoptimized] target(s) in 1.92s Running bin/sqllogictests.rs (target/ci/deps/sqllogictests-c4e4be8d5c9fd66e) Running with 16 test threads (available parallelism: 16) Completed 411 test files in 5 seconds ``` This is actually bounded by the time it takes to run the longest test `push_down_filter_regression.slt`: ```shell andrewlamb@Andrews-MacBook-Pro-3:~/Software/datafusion$ cargo test --profile=ci --test sqllogictests -- push_down_filter_regression.slt Finished `ci` profile [unoptimized] target(s) in 0.20s Running bin/sqllogictests.rs (target/ci/deps/sqllogictests-c4e4be8d5c9fd66e) Running with 16 test threads (available parallelism: 16) Completed 1 test files in 5 seconds ``` ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. -->
1 parent 43584ca commit 02dae77

1 file changed

Lines changed: 68 additions & 3 deletions

File tree

datafusion/sqllogictest/bin/sqllogictests.rs

Lines changed: 68 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@
1818
use clap::{ColorChoice, Parser, ValueEnum};
1919
use datafusion::common::instant::Instant;
2020
use datafusion::common::utils::get_available_parallelism;
21-
use datafusion::common::{DataFusionError, Result, exec_datafusion_err, exec_err};
21+
use datafusion::common::{
22+
DataFusionError, HashMap, Result, exec_datafusion_err, exec_err,
23+
};
2224
use datafusion_sqllogictest::{
2325
CurrentlyExecutingSqlTracker, DataFusion, DataFusionSubstraitRoundTrip, Filter,
2426
TestContext, df_value_validator, read_dir_recursive, setup_scratch_dir,
@@ -47,8 +49,8 @@ use std::fs;
4749
use std::io::{IsTerminal, stderr, stdout};
4850
use std::path::{Path, PathBuf};
4951
use std::str::FromStr;
50-
use std::sync::Arc;
5152
use std::sync::atomic::{AtomicUsize, Ordering};
53+
use std::sync::{Arc, LazyLock};
5254
use std::time::Duration;
5355

5456
#[cfg(feature = "postgres")]
@@ -75,6 +77,55 @@ struct FileTiming {
7577
elapsed: Duration,
7678
}
7779

80+
/// TEST PRIORITY
81+
///
82+
/// Heuristically prioritize some test to run earlier.
83+
///
84+
/// Prioritizes test to run earlier if they are known to be long running (as
85+
/// each test file itself is run sequentially, but multiple test files are run
86+
/// in parallel.
87+
///
88+
/// Tests not listed here will run after the listed tests in an arbitrary order.
89+
///
90+
/// You can find the top longest running tests by running `--timing-summary` mode.
91+
/// For example
92+
///
93+
/// ```shell
94+
/// $ cargo test --profile=ci --test sqllogictests -- --timing-summary top
95+
/// ...
96+
/// Per-file elapsed summary (deterministic):
97+
/// 1. 5.375s push_down_filter_regression.slt
98+
/// 2. 3.174s aggregate.slt
99+
/// 3. 3.158s imdb.slt
100+
/// 4. 2.793s joins.slt
101+
/// 5. 2.505s array.slt
102+
/// 6. 2.265s aggregate_skip_partial.slt
103+
/// 7. 2.260s window.slt
104+
/// 8. 1.677s group_by.slt
105+
/// 9. 0.973s datetime/timestamps.slt
106+
/// 10. 0.822s cte.slt
107+
/// ```
108+
static TEST_PRIORITY: LazyLock<HashMap<PathBuf, usize>> = LazyLock::new(|| {
109+
[
110+
(PathBuf::from("push_down_filter_regression.slt"), 0), // longest running, so run first.
111+
(PathBuf::from("aggregate.slt"), 1),
112+
(PathBuf::from("joins.slt"), 2),
113+
(PathBuf::from("imdb.slt"), 3),
114+
(PathBuf::from("array.slt"), 4),
115+
(PathBuf::from("aggregate_skip_partial.slt"), 5),
116+
(PathBuf::from("window.slt"), 6),
117+
(PathBuf::from("group_by.slt"), 7),
118+
(PathBuf::from("datetime/timestamps.slt"), 8),
119+
(PathBuf::from("cte.slt"), 9),
120+
]
121+
.into_iter()
122+
.collect()
123+
});
124+
125+
/// Default priority for tests not in the TEST_PRIORITY map. Tests with lower
126+
/// priority values run first.
127+
static DEFAULT_PRIORITY: usize = 100;
128+
78129
pub fn main() -> Result<()> {
79130
tokio::runtime::Builder::new_multi_thread()
80131
.enable_all()
@@ -851,7 +902,21 @@ fn read_test_files(options: &Options) -> Result<Vec<TestFile>> {
851902
paths.append(&mut sqlite_paths)
852903
}
853904

854-
Ok(paths)
905+
Ok(sort_tests(paths))
906+
}
907+
908+
/// Sort the tests heuristically by order of "priority"
909+
///
910+
/// Prioritizes test to run earlier if they are known to be long running (as
911+
/// each test file itself is run sequentially, but multiple test files are run
912+
/// in parallel.
913+
fn sort_tests(mut tests: Vec<TestFile>) -> Vec<TestFile> {
914+
tests.sort_by_key(|f| {
915+
TEST_PRIORITY
916+
.get(&f.relative_path)
917+
.unwrap_or(&DEFAULT_PRIORITY)
918+
});
919+
tests
855920
}
856921

857922
/// Parsed command line options

0 commit comments

Comments
 (0)