Skip to content

Conversation

@PsiACE
Copy link
Member

@PsiACE PsiACE commented Oct 26, 2021

Which issue does this PR close?

Closes #897

Rationale for this change

prost and tonic have recently had some updates

What changes are included in this PR?

  • bump deps, like prost 0.9.0, tonic 0.6.0
  • fix some error
  • change to lastest cargo-tarpaulin

Are there any user-facing changes?

I guess no changes

Signed-off-by: Chojan Shang <[email protected]>
Signed-off-by: Chojan Shang <[email protected]>
@github-actions github-actions bot added the arrow-flight Changes to the arrow-flight crate label Oct 26, 2021
@PsiACE PsiACE marked this pull request as draft October 26, 2021 03:39
@PsiACE
Copy link
Member Author

PsiACE commented Oct 26, 2021

Integration Test failed

archery docker run -e ARCHERY_INTEGRATION_WITH_RUST=1 conda-integration

tonic v0.6.0 need rust 1.56+ (edition2021), but it looks https://github.com/apache/arrow/blob/68a081acc4d0e2c85a6469ca165444df6d965d2b/ci/docker/conda-integration.dockerfile#L42 only get 1.55

cc @alamb @nevi-me , I am not familiar with conda-integration, maybe you guys can help me

@PsiACE
Copy link
Member Author

PsiACE commented Oct 26, 2021

also, we need to reset the cache of coverage (cargo and target).

Done.

Signed-off-by: Chojan Shang <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2021

Codecov Report

Merging #864 (aa6251a) into master (1f07735) will decrease coverage by 0.15%.
The diff coverage is 0.00%.

❗ Current head aa6251a differs from pull request most recent head 59dfd6d. Consider uploading reports for the commit 59dfd6d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #864      +/-   ##
==========================================
- Coverage   82.46%   82.30%   -0.16%     
==========================================
  Files         168      168              
  Lines       48175    48093      -82     
==========================================
- Hits        39726    39585     -141     
- Misses       8449     8508      +59     
Impacted Files Coverage Δ
arrow-flight/src/arrow.flight.protocol.rs 0.00% <ø> (ø)
...ng/src/flight_server_scenarios/auth_basic_proto.rs 0.00% <0.00%> (ø)
arrow/src/array/transform/fixed_binary.rs 75.00% <0.00%> (-3.95%) ⬇️
parquet/src/schema/visitor.rs 67.10% <0.00%> (-2.64%) ⬇️
arrow/src/compute/kernels/cast_utils.rs 87.50% <0.00%> (-2.50%) ⬇️
parquet/src/encodings/decoding.rs 90.45% <0.00%> (-2.02%) ⬇️
arrow/src/datatypes/ffi.rs 70.85% <0.00%> (-2.02%) ⬇️
parquet/src/arrow/arrow_reader.rs 89.32% <0.00%> (-1.95%) ⬇️
arrow/src/ffi.rs 84.69% <0.00%> (-1.89%) ⬇️
arrow/src/csv/reader.rs 90.00% <0.00%> (-1.78%) ⬇️
... and 71 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f07735...59dfd6d. Read the comment docs.

@PsiACE PsiACE marked this pull request as ready for review October 26, 2021 15:11
@PsiACE
Copy link
Member Author

PsiACE commented Oct 27, 2021

https://github.com/PsiACE/arrow-rs/runs/4022900917?check_suite_focus=true

re-run Integration Test , and pass all tests

Integration Test failed

archery docker run -e ARCHERY_INTEGRATION_WITH_RUST=1 conda-integration

tonic v0.6.0 need rust 1.56+ (edition2021), but it looks https://github.com/apache/arrow/blob/68a081acc4d0e2c85a6469ca165444df6d965d2b/ci/docker/conda-integration.dockerfile#L42 only get 1.55

https://issues.apache.org/jira/browse/ARROW-14489 , thanks @jimexist

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @PsiACE -- this looks great

@tustvold tustvold mentioned this pull request Nov 1, 2021
@alamb
Copy link
Contributor

alamb commented Nov 1, 2021

So since these are non minor version updates I think this is basically a non-backwards compatible set of changes -- so I can't backport them to the arrow 6.x line, but they'll be released in 7.0

I merged master to make sure that this is still good and will merge if it passes

@alamb alamb added the api-change Changes to the arrow API label Nov 1, 2021
@alamb
Copy link
Contributor

alamb commented Nov 1, 2021

MIRI failure tracked by #879

@alamb
Copy link
Contributor

alamb commented Nov 1, 2021

Merged again to see if we can get a clean passing run

@alamb alamb merged commit 1d3d5e3 into apache:master Nov 2, 2021
@alamb
Copy link
Contributor

alamb commented Nov 2, 2021

Thanks again @PsiACE

@alamb alamb changed the title Bump deps Bump deps to upgrade tonic / prost / prost-derive Nov 9, 2021
alamb added a commit to alamb/arrow-rs that referenced this pull request Nov 11, 2021
* Bump deps

Signed-off-by: Chojan Shang <[email protected]>

* Setup lastest cargo-tarpaulin

Signed-off-by: Chojan Shang <[email protected]>

* Try to use the lastest cargo

Signed-off-by: Chojan Shang <[email protected]>

Co-authored-by: Andrew Lamb <[email protected]>
@PsiACE PsiACE deleted the bump-deps branch November 16, 2023 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-change Changes to the arrow API arrow-flight Changes to the arrow-flight crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update prost and tonic

3 participants