ARROW-10712: [Rust] [DataFusion] Add tests to TPC-H benchmarks #9015
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9015 +/- ##
==========================================
- Coverage 82.87% 82.63% -0.24%
==========================================
Files 201 201
Lines 49739 49912 +173
==========================================
+ Hits 41220 41247 +27
- Misses 8519 8665 +146
Continue to review full report at Codecov.
|
rust/benchmarks/src/bin/tpch.rs
Outdated
| } | ||
|
|
||
| async fn convert_tbl(opt: ConvertOpt) -> Result<()> { | ||
| async fn convert_tbl(opt: ConvertOpt) -> Result<Vec<arrow::record_batch::RecordBatch>> { |
There was a problem hiding this comment.
I would avoid making the type here the same (and return empty vec) to let it compile. You could change the code in main fn instead to not use the expression (not generate let binding / returning so the types don't have to be unified)?
There was a problem hiding this comment.
Ok. I have refactored the main fn to allow the different result signatures as it is definitely better. Thanks 👍
rust/benchmarks/src/bin/tpch.rs
Outdated
|
|
||
| array_value_to_string(column, row_index) | ||
| .ok() | ||
| .unwrap_or_else(|| "???".to_string()) |
There was a problem hiding this comment.
Better to have either a good error message or do normal unwrap if it doesn't occur in test code.
There was a problem hiding this comment.
Thanks for the review. To be honest I had copied that from https://github.com/apache/arrow/blob/master/rust/datafusion/tests/sql.rs#L1387 but i have fixed it to panic in the test.
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks for doing this @seddonm1 . This is a great step forward. I tried to run these tests locally and had some issues -- likely to be doing something incorrectly, and thus I think improving the documentation might be helpful here; I suggest:
- When the test is run without
TPCH_DATAbeing set, display a more prominent warning - Explain clearly how to prepare the data / environment and then how to run the test
When I just ran cargo test everything seemed like it was all fine (which was because the tests were all skipped as I hadn't set TPCH_DATA).
(arrow_dev) alamb@MacBook-Pro:~/Software/arrow2/rust/benchmarks$ cargo test
Finished test [unoptimized + debuginfo] target(s) in 0.14s
Running /Users/alamb/Software/arrow2/rust/target/debug/deps/nyctaxi-527f507403106ec9
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
Running /Users/alamb/Software/arrow2/rust/target/debug/deps/tpch-0d132836f997f03f
running 22 tests
test tests::q10 ... ok
test tests::q14 ... ok
test tests::q3 ... ok
test tests::q12 ... ok
test tests::q17 ... ok
test tests::q21 ... ok
test tests::q15 ... ok
test tests::q11 ... ok
test tests::q19 ... ok
test tests::q20 ... ok
test tests::q22 ... ok
test tests::q13 ... ok
test tests::q18 ... ok
test tests::q2 ... ok
test tests::q1 ... ok
test tests::q16 ... ok
test tests::q4 ... ok
test tests::q5 ... ok
test tests::q6 ... ok
test tests::q7 ... ok
test tests::q8 ... ok
test tests::q9 ... ok
test result: ok. 22 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
(arrow_dev) alamb@MacBook-Pro:~/Software/arrow2/rust/benchmarks$
Then I went and ran tpch-dbgen and set TPCH_DATA
alamb@MacBook-Pro:~/Software/tpch-dbgen$ ./dbgen -vf -s 1
...
(arrow_dev) alamb@MacBook-Pro:~/Software/arrow2/rust/benchmarks$ TPCH_DATA='/Users/alamb/Software/tpch-dbgen' cargo test
The tests failed (this time with wrong answers). An excerpt:
---- tests::q8 stdout ----
Running benchmarks with the following options: BenchmarkOpt { query: 8, debug: false, iterations: 1, concurrency: 2, batch_size: 4096, path: "/Users/alamb/Software/tpch-dbgen", file_format: "tbl", mem_table: false }
Error: Plan("Schema contains duplicate unqualified field name \'n_nationkey\'")
thread 'tests::q8' panicked at 'assertion failed: `(left == right)`
left: `1`,
right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /Users/alamb/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/test/src/lib.rs:192:5
---- tests::q7 stdout ----
Running benchmarks with the following options: BenchmarkOpt { query: 7, debug: false, iterations: 1, concurrency: 2, batch_size: 4096, path: "/Users/alamb/Software/tpch-dbgen", file_format: "tbl", mem_table: false }
Error: Plan("Schema contains duplicate unqualified field name \'n_nationkey\'")
thread 'tests::q7' panicked at 'assertion failed: `(left == right)`
left: `1`,
right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /Users/alamb/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/test/src/lib.rs:192:5
|
@alamb thanks for your feedback. This PR was based on the work @andygrove started here #8760 and was really just the work of loading the tpch-gen answers and adding the comparison code. The original intention of Andy's work was to allow us to automate the tpch-gen process in the CICD testing allowing verification of a real-world test set - and actually quite a wide coverage test. This is why running without TPCH_DATA set does not fail the test - as we probably do not want to enable the automated testing yet nor make all PRs fail testing. If this rationale makes sense I can do some work on the README to make this clear? |
|
I tested this locally and it worked well for me. I also see many test failures, which are to be expected. We may want to make that clear in the docs but I think we can do that as a follow-up. |
5 participants
This PR adds the ability to load and parse the expected query answers included in the https://github.com/databricks/tpch-dbgen/tree/master/answers repository - which users have to clone anyway to generate the TPC-H data.
Currently DataFusion does not support Decimal types which all the numeric values in TPC-H are so there are the expected precision errors in the current results. These tests are still useful as they show some interesting results already such as non-deterministic query 5 results.
@andygrove