Upgrade to arrow 20.0.0 (but no change to object_store), including `prost`, and `tonic` #3083

avantgardnerio · 2022-08-08T19:32:38Z

Closes #3006.

Closes #3085
Closes #3086
Closes #3087

This is an alternative to #3007 but it retains object_store = "0.3.0".

Rationale for this change

We'll have to do it sooner or later, but mostly I want the new authentication capabilities in the FlightSqlServer code.

What changes are included in this PR?

An attempt at upgrading to the latest arrow.

Are there any user-facing changes?

Users should be able to use arrow 20

fix decimal (#4) Fix human error Patch crates io to fix build (#5) * fix decimal * patch crate versions Patch objectstore Test in CI Undo override? Fix more errors Fix last error? Formatting Clippy Fixes Fix refs Able to get session context, but JDBC driver hung Upgrade to arrow 20 Upgrade to RC2 Formatting Fix some imports Install protoc Try platform agnostic path Debug in CI :( Debug in CI :( Debug in CI :( Not worth it, just separate builds Variables Fixes Fix windows? Fix windows? Hackily fix windows Down to 1 failure Fix protoc All? tests pass Formatting

avantgardnerio · 2022-08-08T19:46:39Z

@tustvold here's the other version with object_store = "0.3.0"

tustvold · 2022-08-08T19:49:51Z

👀 This makes no sense, none of this should have changed 😢

tustvold · 2022-08-08T19:53:03Z

Oh, you've made a load of path changes - can you revert these 🙏

tustvold · 2022-08-08T19:53:40Z

datafusion/core/src/datasource/file_format/json.rs

        let format = JsonFormat::default().with_schema_infer_max_rec(Some(3));

+        let store_root = std::path::Path::new(env!("CARGO_MANIFEST_DIR"));
+        let filename = store_root.join("tests/jsons/schema_infer_limit.json");
+        let filename = filename.to_str().expect("Unable to get path!");


Reverting these should make things work

Ah, thank you! I missed these when reverting things. I'm pushing a new commit to this PR that I think will be releasable.

Sorry I missed these, I appreciate your help 🙌

No worries at all, for reference the reason this breaks is that paths on Windows not only don't start with / but when canonicalized get converted to extended length path syntax. This is not a valid object store path, and so it is complaining about this.

Paths on Windows is a complete nonsensical mess, as a result LocalFileSystem actually just implements the file URI standard which basically behaves like paths on Linux (or paths on anything that isn't Windows 😆 ). As such if you feed it something that isn't a file URI path, it will complain

avantgardnerio · 2022-08-08T20:20:37Z

@alamb and @andygrove likely this will be a better release candidate than #3007 for the reasons @tustvold explained above.

codecov-commenter · 2022-08-08T20:39:51Z

Codecov Report

Merging #3083 (be64e65) into master (85d5363) will increase coverage by 0.00%.
The diff coverage is 96.01%.

@@           Coverage Diff           @@
##           master    #3083   +/-   ##
=======================================
  Coverage   85.93%   85.94%           
=======================================
  Files         289      289           
  Lines       52101    52100    -1     
=======================================
+ Hits        44774    44775    +1     
+ Misses       7327     7325    -2

Impacted Files	Coverage Δ
...usion/core/src/avro_to_arrow/arrow_array_reader.rs	`0.00% <0.00%> (ø)`
datafusion/core/src/catalog/information_schema.rs	`94.42% <0.00%> (ø)`
datafusion/core/src/physical_plan/hash_utils.rs	`40.43% <0.00%> (ø)`
datafusion/core/src/physical_plan/repartition.rs	`93.26% <ø> (ø)`
...tafusion/core/src/physical_plan/sort_merge_join.rs	`90.36% <0.00%> (ø)`
datafusion/core/tests/sql/aggregates.rs	`99.36% <ø> (ø)`
datafusion/core/tests/sql/decimal.rs	`100.00% <ø> (ø)`
datafusion/core/tests/sql/joins.rs	`99.33% <ø> (ø)`
datafusion/expr/src/type_coercion.rs	`98.87% <ø> (ø)`
...on/physical-expr/src/expressions/binary/adapter.rs	`83.33% <ø> (ø)`
... and 34 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

avantgardnerio · 2022-08-08T20:44:11Z

@alamb & @andygrove the only remaining failure looks like an intermittent network issue. I think we're GTG here 🎉

alamb · 2022-08-08T21:08:34Z

Great -- I think we are on track for an arrow-rs release in a few hours (I need to wait for the official 72 hour voting period to end) -- 🔜

alamb · 2022-08-08T21:08:41Z

Thanks so much for this @avantgardnerio

alamb

@avantgardnerio do you have time today to update this to use the released arrow 20.0.0? If not I can make a PR (based on this one) to get in.

avantgardnerio · 2022-08-09T15:10:41Z

@avantgardnerio do you have time today to update this to use the released arrow 20.0.0? If not I can make a PR (based on this one) to get in.

I'll do that now.

alamb

This looks like epic work @avantgardnerio -- thanks again

alamb · 2022-08-09T15:13:04Z

CONTRIBUTING.md

@@ -35,6 +35,15 @@ list to help you get started.

 This section describes how you can get started at developing DataFusion.

+### Windows setup
+
+```shell


Is this still correct? Does it need any additional explination?

I just put that in there based upon the setup of a VM I did yesterday. I do tend to like scripts-as-doc as it eliminates the ambiguity of English. Ideally CI would start with a vanilla Windows machine and run these scripts, but I guess github did that part for us so we just have to doc it some where.

I think it is valuable -- thank you

alamb · 2022-08-09T15:14:10Z

datafusion/core/Cargo.toml

-datafusion-jit = { path = "../jit", version = "10.0.0", optional = true }
-datafusion-optimizer = { path = "../optimizer", version = "10.0.0" }
-datafusion-physical-expr = { path = "../physical-expr", version = "10.0.0" }
-datafusion-row = { path = "../row", version = "10.0.0" }


I can't remember if the versions are needed to cargo publish these crates to crates.io -- perhaps @andygrove remembers / knows?

alamb

Looks good to me -- 🤞 for a clean CI run

avantgardnerio · 2022-08-09T15:36:35Z

Looks good to me -- crossed_fingers for a clean CI run

I ran all the checks locally and they passed ... so 🤞

alamb · 2022-08-09T16:45:18Z

🚀

alamb · 2022-08-09T16:45:31Z

Thanks again everyone for all the work to get this through

ursabot · 2022-08-09T16:52:29Z

Benchmark runs are scheduled for baseline = 1e44417 and contender = 3eb55e9. 3eb55e9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Brent Gardner added 4 commits August 8, 2022 09:50

Fix remaining tests

5270354

Clippy

db9899e

Update docs for Windows

092e449

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Physical Expressions sql SQL Planner labels Aug 8, 2022

avantgardnerio mentioned this pull request Aug 8, 2022

Upgrade to arrow 20.0.0 #3007

Closed

tustvold reviewed Aug 8, 2022

View reviewed changes

Try with old objectstore

2761a7e

Revert path "fixes" that broke windows

be64e65

alamb reviewed Aug 9, 2022

View reviewed changes

Update to arrow 20

fd9e6d6

alamb approved these changes Aug 9, 2022

View reviewed changes

alamb changed the title ~~Upgrade to arrow 20.0.0 (but no change to object_store)~~ Upgrade to arrow 20.0.0 (but no change to object_store), including prost, and tonic Aug 9, 2022

alamb merged commit 3eb55e9 into apache:master Aug 9, 2022

avantgardnerio deleted the bg_old_objectstore branch August 9, 2022 16:49

This was referenced Aug 9, 2022

Release DataFusion 11.0.0 #3012

Closed

WIP: Update prost/tonic #3029

Closed

Upgrade prost/tonic everywhere #3028

Closed

Ted-Jiang mentioned this pull request Aug 29, 2022

update datafusion and arrow to 20.0.0 apache/datafusion-ballista#176

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to arrow 20.0.0 (but no change to object_store), including `prost`, and `tonic` #3083

Upgrade to arrow 20.0.0 (but no change to object_store), including `prost`, and `tonic` #3083

avantgardnerio commented Aug 8, 2022 •

edited by alamb

Loading

avantgardnerio commented Aug 8, 2022

tustvold commented Aug 8, 2022

tustvold commented Aug 8, 2022

tustvold Aug 8, 2022

avantgardnerio Aug 8, 2022

tustvold Aug 8, 2022 •

edited

Loading

avantgardnerio commented Aug 8, 2022

codecov-commenter commented Aug 8, 2022

avantgardnerio commented Aug 8, 2022

alamb commented Aug 8, 2022

alamb commented Aug 8, 2022

alamb left a comment

avantgardnerio commented Aug 9, 2022

alamb left a comment

alamb Aug 9, 2022

avantgardnerio Aug 9, 2022

alamb Aug 9, 2022

alamb Aug 9, 2022

alamb left a comment

avantgardnerio commented Aug 9, 2022

alamb commented Aug 9, 2022

alamb commented Aug 9, 2022

ursabot commented Aug 9, 2022

Upgrade to arrow 20.0.0 (but no change to object_store), including prost, and tonic #3083

Upgrade to arrow 20.0.0 (but no change to object_store), including prost, and tonic #3083

Conversation

avantgardnerio commented Aug 8, 2022 • edited by alamb Loading

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

avantgardnerio commented Aug 8, 2022

tustvold commented Aug 8, 2022

tustvold commented Aug 8, 2022

tustvold Aug 8, 2022

Choose a reason for hiding this comment

avantgardnerio Aug 8, 2022

Choose a reason for hiding this comment

tustvold Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

avantgardnerio commented Aug 8, 2022

codecov-commenter commented Aug 8, 2022

Codecov Report

avantgardnerio commented Aug 8, 2022

alamb commented Aug 8, 2022

alamb commented Aug 8, 2022

alamb left a comment

Choose a reason for hiding this comment

avantgardnerio commented Aug 9, 2022

alamb left a comment

Choose a reason for hiding this comment

alamb Aug 9, 2022

Choose a reason for hiding this comment

avantgardnerio Aug 9, 2022

Choose a reason for hiding this comment

alamb Aug 9, 2022

Choose a reason for hiding this comment

alamb Aug 9, 2022

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

avantgardnerio commented Aug 9, 2022

alamb commented Aug 9, 2022

alamb commented Aug 9, 2022

ursabot commented Aug 9, 2022

Upgrade to arrow 20.0.0 (but no change to object_store), including `prost`, and `tonic` #3083

Upgrade to arrow 20.0.0 (but no change to object_store), including `prost`, and `tonic` #3083

avantgardnerio commented Aug 8, 2022 •

edited by alamb

Loading

tustvold Aug 8, 2022 •

edited

Loading