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

Triage remaining integration test failures with other Arrow implementations #1404

Closed
alamb opened this issue Mar 5, 2022 · 10 comments
Closed
Labels
arrow Changes to the arrow crate help wanted

Comments

@alamb
Copy link
Contributor

alamb commented Mar 5, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
It would be nice to have more complete integration coverage for the rust implementation, as well as more documentation and community knowledge about how to run them.

re #1398

The basic idea here is to triage remaining failures in the integration tests with the rust implementation and file tickets and/or fix them.

This is a nice project for people to work on in their spare time and would likely expose them to parts of the arrow crate.

Describe the solution you'd like

  1. Setup the integration tests as described in Improve integration testing docs #1403
  2. Verify that running archery --debug integration --run-flight --with-cpp=true --with-rust=true passes without error

Then, for each line like .skip_category('Rust'), in https://github.com/apache/arrow/blob/master/dev/archery/archery/integration/datagen.py,:

  1. remove it from datagen.py
  2. Rerun archery --debug integration --run-flight --with-cpp=true --with-rust=true
  3. File figure out why it fails and fix it / file a ticket

Additional context
re #1398

@alamb alamb added enhancement Any new improvement worthy of a entry in the changelog help wanted arrow Changes to the arrow crate and removed enhancement Any new improvement worthy of a entry in the changelog labels Mar 5, 2022
@viirya
Copy link
Member

viirya commented Mar 15, 2022

@viirya
Copy link
Member

viirya commented Mar 15, 2022

I will work on above list one by one.

@viirya
Copy link
Member

viirya commented Apr 10, 2022

generate_nested_large_offsets_case seems a false negative case. The test can be passed on current master without any change.

@alamb
Copy link
Contributor Author

alamb commented Apr 11, 2022

generate_nested_large_offsets_case seems a false negative case. The test can be passed on current master without any change.

Perhaps we have fixed it in some previous PR

@viirya
Copy link
Member

viirya commented May 21, 2022

For generate_extension_case, although there is .skip_category('Rust') but running archery --debug integration --run-flight --with-cpp=true --with-rust=true is okay without any failure even removing the skip_category.

I don't find any extension type support in current codebase, so I suppose it is not supported.

I guess it is passed because IPC reader/writer already correctly process custom metadata in IPC message, so the metadata is preserved well.

We just don't have an explicit extension type support. But running the integration test is okay as we can read/write the extension metadata correctly to talk with C++ Arrow.

@viirya
Copy link
Member

viirya commented May 21, 2022

Okay, I fixed all integration test failures except for generate_decimal256_case. Currently the only failing test case is generate_decimal256_case which needs #131. Not sure if it is straightforward to add the Decimal 256 support, but I may take some time looking into it.

Do we need to send a patch to dev/archery/archery/integration/datagen.py in C++ Arrow repo to enable these Rust cases?

@alamb
Copy link
Contributor Author

alamb commented May 23, 2022

Okay, I fixed all integration test failures

❤️ thank you so much @viirya !!!!

except for generate_decimal256_case. Currently the only failing test case is generate_decimal256_case which needs #131. Not sure if it is straightforward to add the Decimal 256 support, but I may take some time looking into it.

Nice -- I haven't heard anyone else asking for Decimal256 support but it comes up occasionally (https://github.com/apache/arrow-rs/search?q=decimal256&type=issues) and would be great to round out support.

Do we need to send a patch to dev/archery/archery/integration/datagen.py in C++ Arrow repo to enable these Rust cases?

Yes, I think so -- apache/arrow#11238 show an example of such a PR

alamb pushed a commit to apache/arrow that referenced this issue Jun 11, 2022
…13219)

arrow-rs has fixed severals integration test failures (apache/arrow-rs#1404):

generate_decimal128_case
generate_interval_case
generate_map_case
generate_non_canonical_map_case
generate_nested_large_offsets_case
generate_nested_dictionary_case
generate_unions_case

And this one passes test without any fix:
generate_extension_case

We should activate these IPC integration tests for rust.

Authored-by: Liang-Chi Hsieh <viirya@gmail.com>
Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
@viirya
Copy link
Member

viirya commented Jun 11, 2022

As we enable almost all test cases except for generate_decimal256_case which we have an issue #131 tracking it, maybe we can close this.

cc @alamb

@alamb
Copy link
Contributor Author

alamb commented Jun 12, 2022

I agree -- great job @viirya -- your work on this issue was epic. Thank you again

@alamb alamb closed this as completed Jun 12, 2022
@viirya
Copy link
Member

viirya commented Jun 12, 2022

Thank you @alamb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate help wanted
Projects
None yet
Development

No branches or pull requests

2 participants