-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add golden table tests from delta spark/java kernel #295
Add golden table tests from delta spark/java kernel #295
Conversation
test-log = { version = "0.2", default-features = false, features = ["trace"] } | ||
tempfile = "3" | ||
test-case = { version = "3.1.0" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like it wasn't used?
@@ -387,14 +381,6 @@ macro_rules! assert_batches_sorted_eq { | |||
}; | |||
} | |||
|
|||
fn to_arrow(data: Box<dyn EngineData>) -> DeltaResult<RecordBatch> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to common
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, this is much better!
Left some comments, but mostly looking good, thanks.
full_scan_test!("multi-part-checkpoint"); | ||
full_scan_test!("only-checkpoint-files"); | ||
|
||
// TODO some of the parquet tests use projections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we support projections, so why can't we do these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea I think with your comment below I'll just try to unify all the tests under a golden_table!
macro and we can just always pass in a 'test function' and I'll make one for full table scan, scan with predicate, etc. (or maybe a function that has that stuff as params
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
taking as follow-up. I can make issues for some of these TODOs?
skip_test!("data-reader-timestamp_ntz-id-mode": "id column mapping mode not supported"); | ||
full_scan_test!("data-reader-timestamp_ntz-name-mode"); | ||
|
||
// TODO test with predicate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you elaborate a bit on what exactly we need to do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as comment above I think i'll be able to implement these - but will size how much work is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually going to just punt on these for a separate PR just so we can get this merged and iterate
kernel/tests/golden_tables.rs
Outdated
Ok(Some(all_data)) | ||
} | ||
|
||
// TODO: change to do something similar to dat tests instead of string comparison |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: explain what dat does. i.e. "change to use arrow's column Eq
like dat does, instead of string comparison. Should print out string when test fails to make debugging easier"
This could be a good first issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea i can go through some of these todos and make explicit issues?
todo find/profile the long test |
golden_parquet_decimal_dictionaries* tests |
slowness was due to printing tables for comparison. the large tables in particular in the decimal tests. fixed now to be smart and just clear metadata from record batches and then compare sorted record batches with existing Eq implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking pretty good, just a few last things
kernel/tests/golden_tables.rs
Outdated
.map(normalize_col) | ||
.collect::<Vec<_>>(); | ||
|
||
let left: RecordBatch = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're creating a record batch here out of the normalized cols, then immediately deconstructing it into columns again to sort them.
I think you should be able to just pass the columns as a slice to sort_record_batch
(and probably rename it sort_columns
or similar).
Alternately, do it more like in DAT, where you first sort the cols, then iterate over each column, normalize it, and then check that it's equal. That let's you be a bit nicer in the error message:
fn assert_columns_match(actual: &[Arc<dyn Array>], expected: &[Arc<dyn Array>]) {
for (actual, expected) in actual.iter().zip(expected) {
let actual = normalize_col(actual.clone());
let expected = normalize_col(expected.clone());
// note that array equality includes data_type equality
// See: https://arrow.apache.org/rust/arrow_data/equal/fn.equal.html
assert_eq!(
&actual, &expected,
"Column data didn't match. Got {actual:?}, expected {expected:?}"
);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm doing this to rely on the schema + cols check for the whole record batch. seemed like DAT was doing something less clean with checking schema and data equality separately? but yea wonder if I should descruct, normalize, sort, then put back together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! we do need to exclude (fixing I assume is a bit more work) some more tests, but otherwise should give us a great dela more confidence things work as they should!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! thanks for iterating on this, and for the couple of critical bugfixes!
Adding delta's existing golden table tests to the kernel integration test suite on a best-effort basis. Each golden table is a
.tar.zst
compressed folder of the table (test input) and expected data (to assert proper reads) - all are inkernel/tests/golden_data/
.Each golden-table.tar.zst is organized like:
The golden tests persisted in the delta repo are the 'input' of the test cases - i.e. the delta tables we would like to read (under
delta/
subdirectory.Expected outputs were generated by reading the latest snapshot of each table from above with PySpark then persisted in the
expected/
subdirectory.A new integration suite
kernel/tests/golden_tables.rs
runs each of the new tests based on a set of macros to run either (1)golden_test!
: a test function against the golden table, (2)negative_test!
: run the test with the latest snapshot and expect it to fail, (3)skip_test!
: skip the test with a reasonrun the new tests with
cargo t -p delta_kernel --test golden_tables