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

ARROW-16636: [Rust] Activate several IPC integration tests for rust #13219

Merged
merged 4 commits into from
Jun 11, 2022

Conversation

viirya
Copy link
Member

@viirya viirya commented May 23, 2022

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.

@github-actions
Copy link

@github-actions
Copy link

⚠️ Ticket has no components in JIRA, make sure you assign one.

@github-actions
Copy link

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@viirya
Copy link
Member Author

viirya commented May 23, 2022

cc @alamb

@viirya
Copy link
Member Author

viirya commented May 23, 2022

Got the following errors in CI:

################# FAILURES #################
FAILED TEST: nested_large_offsets Java producing,  Rust consuming

FAILED TEST: decimal Rust producing,  Rust consuming

FAILED TEST: union Rust producing,  Rust consuming

Hmm, I ran archery integration --with-cpp=true --with-rust=true locally without any issue. We also don't run Java. I'll look into the difference.

@alamb
Copy link
Contributor

alamb commented May 24, 2022

This is epic -- thank you @viirya for looking into this

@viirya
Copy link
Member Author

viirya commented May 28, 2022

I'm in business trip this week. I will have more time to look into this in next week.

@viirya
Copy link
Member Author

viirya commented May 31, 2022

Regarding the failure FAILED TEST: decimal Rust producing, Rust consuming, it is because Rust performs a decimal check that C++ doesn't. And in the golden file 0.14.1_decimal.gold.json, there are some values failing this check. I can conform that the test can be passed if removing the check in Rust side.

I propose a quick fix at Rust side to remove it, just following C++ Arrow decimal builder.

@viirya
Copy link
Member Author

viirya commented Jun 3, 2022

Yea, FAILED TEST: decimal Rust producing, Rust consuming was fixed now. Continuing looking into remaining two failed cases.

@viirya
Copy link
Member Author

viirya commented Jun 4, 2022

For FAILED TEST: union Rust producing, Rust consuming, proposed the fix at apache/arrow-rs#1789

@viirya
Copy link
Member Author

viirya commented Jun 9, 2022

@alamb All tests are passed now. Please take a look. Thanks.

@alamb
Copy link
Contributor

alamb commented Jun 9, 2022

Amazing work @viirya ❤️

@alamb
Copy link
Contributor

alamb commented Jun 9, 2022

The arrow repo used to have a different way of merging PRs (using a script) -- let me know if you would like to give it a try or if you would like me to. 🎖️

@viirya
Copy link
Member Author

viirya commented Jun 9, 2022

@alamb Thank you. I may rarely touch C++ repo. Please help merging this. If I have more works in this repo in the future, I can catch up it later.

@alamb
Copy link
Contributor

alamb commented Jun 11, 2022

Looking into it

@alamb
Copy link
Contributor

alamb commented Jun 11, 2022

Following the process in https://github.com/apache/arrow/tree/master/dev

@alamb alamb merged commit 42a2efd into apache:master Jun 11, 2022
@alamb
Copy link
Contributor

alamb commented Jun 11, 2022

Thanks again @viirya -- this is a major step forward

@viirya
Copy link
Member Author

viirya commented Jun 11, 2022

Thank you @alamb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants