You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/05/28 15:39:50 UTC
[GitHub] [arrow] nealrichardson opened a new pull request #7297: Try to run rust integration tests
nealrichardson opened a new pull request #7297:
URL: https://github.com/apache/arrow/pull/7297
FYI @andygrove. I looked at the integration test output and saw that Rust tests weren't being run at all, so I'm trying to enable them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7297: ARROW-6945: [Rust][Integration] Run rust integration tests
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-647573600
@nevi-me @nealrichardson I have some time available this week. How can I help with this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #7297: ARROW-6945: [Rust][Integration] Run rust integration tests
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-642223515
@andygrove @nealrichardson Rust tests are now running (binary found). The problem was that we were setting the build output directory to `build/rust/target/...` instead of the default `arrow/rust/target/...` folder. Then in the integration tests we were looking for the Rust binary in the latter location.
I've removed the part of the build script in `ci/scripts/rust_build.sh` that did the above, and unit tests are also still running correctly. Should I go ahead and remove the lines that I commented out?
Neal, I'm not sure of what else would need to be done for the integration tests to run on each commit/PR. Otherwise I think this is good to merge.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #7297: ARROW-6945: [Rust][Integration] Run rust integration tests
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-642206959
> Hi @andygrove @nevi-me, what do we need to do to merge this and get Rust integration tests (at least those that are currently supported) running on all commits?
I think I've found the problem with the Rust binary not being found, I hope you don't mind me tinkering directly on your branch @nealrichardson. We can merge this after I fix the problem.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7297: ARROW-6945: [Rust] [Integration Tests] Try to run rust integration tests
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-635747177
I used docker compose to run this locally and could not reproduce the issue. The tests ran for me.
Brief sample output:
```
["/arrow/rust/target/debug/arrow-json-integration-test", "--integration", "--arrow=/tmp/tmpuv1jy3t6/8766f48c_generated_nested_dictionary.json_as_file", "--json=/tmp/arrow-integration-ewn5k3l5/generated_nested_dictionary.json", "--mode=JSON_TO_ARROW"]
Converting /tmp/arrow-integration-ewn5k3l5/generated_nested_dictionary.json to /tmp/tmpuv1jy3t6/8766f48c_generated_nested_dictionary.json_as_file
Error: JsonError("data type Dictionary(Int8, List(Dictionary(Int8, Utf8))) not supported")
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #7297: Try to run rust integration tests
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-635486083
Did you say that integration tests were a separate crate now? Maybe that wasn't built. I just added `ci/scripts/rust_build.sh`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #7297: Try to run rust integration tests
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-635471171
Ok @andygrove I got them running (in a sense) but they all fail. See https://github.com/apache/arrow/pull/7297/checks?check_run_id=717518551
Error message is all `FileNotFoundError: [Errno 2] No such file or directory: '/arrow/rust/target/debug/arrow-json-integration-test'` so something isn't getting set up correctly. I'm hoping you know what that is. Can you take over from here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7297: Try to run rust integration tests
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-635431612
Thanks @nealrichardson .. so far I've just been running them manually with (from memory) `archery integration --with-java=true --with-rust=true`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me edited a comment on pull request #7297: ARROW-6945: [Rust][Integration] Run rust integration tests
Posted by GitBox <gi...@apache.org>.
nevi-me edited a comment on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-647578097
@andygrove thanks. I pushed some of what I worked on over the weekend. The main problem seems to be that we don't read all record batches from the Arrow files. As a result we end up getting "no more record batches" errors.
I haven't yet been able to figure out why we don't read all batches, the block sizes indicate the correct batch counts, but the reader just fails to read them all 😞
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #7297: ARROW-6945: [Rust][Integration] Run rust integration tests
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#discussion_r443641664
##########
File path: dev/archery/archery/integration/datagen.py
##########
@@ -1492,21 +1492,25 @@ def _temp_path():
generate_primitive_large_offsets_case([17, 20])
.skip_category('Go')
- .skip_category('JS'),
+ .skip_category('JS')
+ .skip_category('Rust'),
generate_null_case([10, 0])
+ .skip_category('Rust')
Review comment:
Didn't I see a patch merge recently that added null support? If you rebase, can you remove this skip?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #7297: ARROW-6945: [Rust][Integration] Run rust integration tests
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-642836792
@nevi-me @andygrove is it expected that everything except `primitive_no_batches` fails for Rust? If so, we can skip everything else, and y'all can unskip them iteratively when you get to them. If that's not expected, you should probably debug that before merging this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #7297: ARROW-6945: [Rust] [Integration Tests] Try to run rust integration tests
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-637103723
I have made progress on large arrays (https://github.com/apache/arrow/compare/master...nevi-me:rust-large-lists), the code is very repetitive so I still need to clean it up.
When large lists are implemented, there are 2 issues that become visible:
- We don't handle empty batch tests correctly (we fail instead of returning something)
- There's a bug with how we write `NullArray` which results in any arrays that are written after, not being read by the consumer.
I think outside of the above, we can disable other tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7297: ARROW-6945: [Rust] [Integration Tests] Try to run rust integration tests
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-635675365
https://issues.apache.org/jira/browse/ARROW-6945
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #7297: Try to run rust integration tests
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-635485426
Side note: does this need a new JIRA or does one already exist. Maybe use https://issues.apache.org/jira/browse/ARROW-6945 for this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7297: ARROW-6945: [Rust][Integration] Run rust integration tests
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-644505701
@nealrichardson I think that would be a reasonable assumption to make at this point. I'd be happy with this merged even if only a small number of tests are passing and we can iterate from there, Sorry, I'd help more but am working long hours in the day job currently. I should have time before the release to re-engage with this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7297: ARROW-6945: [Rust] [Integration Tests] Try to run rust integration tests
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-635988046
Full logs are attached to the JIRA
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #7297: ARROW-6945: [Rust][Integration] Run rust integration tests
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-648900443
Ok there was one other test needing to be skipped, which I've done, and now the tests "pass". Should we merge this and progressively unskip tests as you can?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7297: ARROW-6945: [Rust][Integration] Run rust integration tests
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-648901377
Yes, that would be great. Thanks!
On Wed, Jun 24, 2020 at 9:43 AM Neal Richardson <no...@github.com>
wrote:
> Ok there was one other test needing to be skipped, which I've done, and
> now the tests "pass". Should we merge this and progressively unskip tests
> as you can?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/arrow/pull/7297#issuecomment-648900443>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAHEBRHANUIKCVMDCVFPLKLRYININANCNFSM4NNGK6YA>
> .
>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #7297: ARROW-6945: [Rust][Integration] Run rust integration tests
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-648901870
> Ok there was one other test needing to be skipped, which I've done, and now the tests "pass". Should we merge this and progressively unskip tests as you can?
Yes please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7297: Try to run rust integration tests
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-635483439
Thanks @nealrichardson .. `cargo build` looks like it ran ok, so I don't know why the binary doesn't exist in that location (which looks correct). I will take a look at this tonight and see what the issue is.
cc @vertexclique Since you were interested in helping with this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7297: Try to run rust integration tests
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-635431357
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson closed pull request #7297: ARROW-6945: [Rust][Integration] Run rust integration tests
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #7297:
URL: https://github.com/apache/arrow/pull/7297
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #7297: ARROW-6945: [Rust][Integration] Run rust integration tests
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-642240631
I think https://github.com/apache/arrow/pull/7297/files#diff-7a8805436a6884ddf74fe3eaec697e71R624 is sufficient to get them to run on each commit/PR.
If you've commented out unnecessary lines from the rust_build.sh script and things run correctly without them, I would guess you can delete them, but I don't know the specifics.
Judging from the test output, there are some more skips that need to be added to https://github.com/apache/arrow/blob/master/dev/archery/archery/integration/datagen.py#L1488, or maybe there's some other reason they're failing.
Feel free to keep pushing to this branch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #7297: ARROW-6945: [Rust][Integration] Run rust integration tests
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-647579286
Oh, there's also a TODO on array data comparisons. I think we only compare the array lengths and types for now; but not their data.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #7297: ARROW-6945: [Rust][Integration] Run rust integration tests
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-647578097
@andygrove thanks. I pushed done if what I worked on over the weekend. The main problem seems to be that we don't read all record batches from the Arrow files. As a result we end up getting "no more record batches" errors.
I haven't yet been able to figure out why we don't read all batches, the block sizes indicate the correct batch counts, but the reader just fails to read them all 😞
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson edited a comment on pull request #7297: Try to run rust integration tests
Posted by GitBox <gi...@apache.org>.
nealrichardson edited a comment on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-635485426
Side note: does this need a new JIRA or does one already exist? Maybe use https://issues.apache.org/jira/browse/ARROW-6945 for this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7297: Try to run rust integration tests
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-635488152
Yes it is a new crate but part of the workspace so the top level `cargo build` should build it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #7297: ARROW-6945: [Rust] [Integration Tests] Try to run rust integration tests
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-642118355
Hi @andygrove @nevi-me, what do we need to do to merge this and get Rust integration tests (at least those that are currently supported) running on all commits?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7297: ARROW-6945: [Rust] [Integration Tests] Try to run rust integration tests
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7297:
URL: https://github.com/apache/arrow/pull/7297#issuecomment-635747636
I'm running this again, redirecting output to a log file. I will find a way to share this tomorrow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org