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

test: add data acceptance tests #1551

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

microcassidy
Copy link

Description

Adds data acceptance tests

Related Issue(s)

Documentation

delta-incubator/dat

@github-actions github-actions bot added binding/rust Issues for the Rust crate rust labels Jul 21, 2023
@github-actions
Copy link

ACTION NEEDED

delta-rs follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

@microcassidy microcassidy changed the title add dat testing test: add data acceptance tests Jul 21, 2023
@rtyler
Copy link
Member

rtyler commented Jul 21, 2023

Thanks for the pull request! DAT should not be added directly to the tree, since they should be changing relatively frequently as the protocol evolves. Here's a build.rs from another repository which I think you could incorporate into your pull request to dynamically fetch the latest dat release

@microcassidy
Copy link
Author

Thanks @rtyler, I will fix it up prior to requesting review! I just left it there because I hadn't done the setup part yet and wanted to make it easy for others to take a look at.

Should the build.rs drop the files on exit? or is the intention to leave them in the tree, but just add the data in an automated way?

add dat testing

cleaning up

continuing cleanup

assertions on table version being accessed

add setup script for dat file download
@rtyler
Copy link
Member

rtyler commented Jul 21, 2023

Should the build.rs drop the files on exit? or is the intention to leave them in the tree, but just add the data in an automated way?

I don't believe there is a need to drop the dat tests in an automated way, I think it's best to avoid re-downloading the test release unless necessary.

@rtyler rtyler self-assigned this Jul 21, 2023
@microcassidy
Copy link
Author

microcassidy commented Jul 25, 2023

@rtyler
I have updated the tests with your suggestion. I am encountering an issue on two of the unit tests

running 7 tests
test test_with_schema_change ... ok
test test_with_checkpoint ... ok
test test_stats_as_struct ... ok
test test_no_replay ... ok
test test_basic_partitioned ... FAILED
test test_basic_append ... ok
test test_all_primitive_types ... FAILED

failures:

---- test_basic_partitioned stdout ----
Error: External(External(External(ArrowError(InvalidArgumentError("Column 'CAST(actual.letter AS Utf8)' is declared as non-nullable but contains null values")))))

-This one is unexpected.-

---- test_all_primitive_types stdout ----
Error: External(Execution("Fail to build join indices in HashJoinExec, error:Arrow error: Cast error: Cannot compare two arrays of different types (Timestamp(Nanosecond, None) and Timestamp(Microsecond, None))"))

This one is expected. What is the simplest way to coerce types?
Edit: resolved

Tasks:

  • test test_basic_partitioned
  • test test_all_primitive_types

@hntd187
Copy link
Collaborator

hntd187 commented Nov 1, 2023

Let me look through this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/rust Issues for the Rust crate rust
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add DAT tests for Rust
3 participants