Review use of panic in datafusion-proto crate #3365

comphead · 2022-09-05T18:18:41Z

Which issue does this PR close?

Closes #3318.

Rationale for this change

Refactor to get rid of explicit panics

What changes are included in this PR?

Are there any user-facing changes?

# Conflicts: # datafusion/proto/build.rs

codecov-commenter · 2022-09-05T18:47:45Z

Codecov Report

Merging #3365 (625fcf0) into master (4c948a3) will increase coverage by 0.00%.
The diff coverage is 51.16%.

@@           Coverage Diff            @@
##           master    #3365    +/-   ##
========================================
  Coverage   85.51%   85.51%            
========================================
  Files         294      296     +2     
  Lines       54120    54300   +180     
========================================
+ Hits        46279    46433   +154     
- Misses       7841     7867    +26

Impacted Files	Coverage Δ
datafusion/proto/src/bytes/mod.rs	`82.75% <ø> (ø)`
datafusion/proto/src/from_proto.rs	`34.26% <ø> (ø)`
datafusion/proto/src/logical_plan.rs	`17.46% <0.00%> (-0.35%)`	⬇️
datafusion/proto/src/to_proto.rs	`48.25% <53.65%> (-0.64%)`	⬇️
datafusion/expr/src/udaf.rs	`37.50% <0.00%> (-12.50%)`	⬇️
...ion/physical-expr/src/aggregate/approx_distinct.rs	`50.00% <0.00%> (-9.53%)`	⬇️
benchmarks/src/bin/tpch.rs	`37.59% <0.00%> (-3.56%)`	⬇️
datafusion/physical-expr/src/planner.rs	`94.19% <0.00%> (-1.86%)`	⬇️
datafusion/expr/src/logical_plan/display.rs	`90.12% <0.00%> (-1.02%)`	⬇️
datafusion/expr/src/binary_rule.rs	`84.50% <0.00%> (-0.48%)`	⬇️
... and 44 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

alamb · 2022-09-06T14:56:08Z

Looks like there is a CI error on this PR.

datafusion/proto/src/logical_plan.rs

andygrove · 2022-09-06T18:58:49Z

datafusion/proto/src/to_proto.rs

@@ -224,10 +224,12 @@ impl From<&DataType> for protobuf::arrow_type::ArrowTypeEnum {
                fractional: *fractional as u64,
            }),
            DataType::Decimal256(_, _) => {
-                unimplemented!("The Decimal256 data type is not yet supported")
+                unimplemented!("Proto serialization error: The Decimal256 data type is not yet supported")


If we used impl TryFrom instead of impl From, we could return errors here. This could be done as a separate Pr.

Created #3401

datafusion/proto/src/to_proto.rs

andygrove · 2022-09-06T18:59:58Z

Thanks @comphead. This is looking good. I left some comments.

comphead · 2022-09-07T01:18:23Z

@andygrove @alamb I dont get why cargo check keeps failing.... even on master branch.

alamb · 2022-09-07T11:44:37Z

@andygrove @alamb I dont get why cargo check keeps failing.... even on master branch.

I think the reason that https://github.com/apache/arrow-datafusion/runs/8218776681?check_suite_focus=true is failing is that there is some issue in code that is not normally run via cargo test.

In this case, the command that is failing is:

cargo check --workspace --benches --features avro,jit,scheduler,json

Does that fail locally for you?

comphead · 2022-09-07T15:31:44Z

Thanks @alamb
cargo test works fine locally. but cargo check --workspace --benches --features json does not even switching to main branch. Like some local cache exists. I'll try to get the repo clean copy

comphead · 2022-09-07T21:51:00Z

Sorry guys, I probably generated more commits than expected, the reason being is cargo check gives me not the same results as CI, even after cleaning the cache

andygrove

LGTM. Thanks @comphead

andygrove · 2022-09-07T23:44:59Z

@avantgardnerio I know you fixed a json proto build issue recently. Could you take a look at this PR to make sure you are happy with it?

avantgardnerio

I added a few minor nits, but overall I think this is a definite improvement - thank you for putting in this work!

datafusion/proto/src/from_proto.rs

datafusion/proto/src/logical_plan.rs

avantgardnerio · 2022-09-08T15:07:06Z

datafusion/proto/build.rs

@@ -32,7 +32,9 @@ fn main() -> Result<(), String> {
 fn build() -> Result<(), String> {
    use std::io::Write;

-    let out = std::path::PathBuf::from(std::env::var("OUT_DIR").unwrap());
+    let out = std::path::PathBuf::from(
+        std::env::var("OUT_DIR").expect("Cannot find OUT_DIR environment vairable"),


expect seems semi-reasonable in tests or build.rs

Imho expect here gives more user friendly message than .unwrap with very generic text

expect is a definite improvement to unwrap in my opinion too

comphead · 2022-09-08T17:14:08Z

Thanks @avantgardnerio for the review, please find the fixed PR

alamb

LGTM -- thanks @avantgardnerio and @comphead !

ursabot · 2022-09-08T19:53:16Z

Benchmark runs are scheduled for baseline = 4258751 and contender = 1b40ffa. 1b40ffa 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

comphead added 3 commits September 5, 2022 09:38

removed panics from proto crate

dddaefe

Merge branch 'master' into proto-no-panic

7f0dee3

# Conflicts: # datafusion/proto/build.rs

fixes

0c6c593

comphead changed the title ~~Proto no panic~~ Review use of panic in datafusion-proto crate Sep 5, 2022

andygrove reviewed Sep 6, 2022

View reviewed changes

datafusion/proto/src/logical_plan.rs Outdated Show resolved Hide resolved

andygrove reviewed Sep 6, 2022

View reviewed changes

datafusion/proto/src/to_proto.rs Outdated Show resolved Hide resolved

comphead marked this pull request as draft September 7, 2022 00:01

comphead added 3 commits September 6, 2022 17:13

fixed comments

227e0e0

fix build

713c5a0

fixed comments

38afffd

comphead added 3 commits September 7, 2022 10:44

Fixing proto json

3512cdd

formatting

8138d9a

fix build

4e6e920

comphead marked this pull request as ready for review September 7, 2022 21:39

andygrove approved these changes Sep 7, 2022

View reviewed changes

avantgardnerio approved these changes Sep 8, 2022

View reviewed changes

addresed comments

625fcf0

comphead mentioned this pull request Sep 8, 2022

Replace From trait with TryFrom trait for datafusion-proto crate #3401

Closed

alamb approved these changes Sep 8, 2022

View reviewed changes

alamb merged commit 1b40ffa into apache:master Sep 8, 2022

comphead deleted the proto-no-panic branch September 8, 2022 23:02

comphead mentioned this pull request Sep 10, 2022

Replace From trait with TryFrom trait for datafusion-proto crate #3401 #3429

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review use of panic in datafusion-proto crate #3365

Review use of panic in datafusion-proto crate #3365

comphead commented Sep 5, 2022

codecov-commenter commented Sep 5, 2022 •

edited

Loading

alamb commented Sep 6, 2022

andygrove Sep 6, 2022

comphead Sep 8, 2022 •

edited

Loading

andygrove commented Sep 6, 2022

comphead commented Sep 7, 2022

alamb commented Sep 7, 2022

comphead commented Sep 7, 2022

comphead commented Sep 7, 2022

andygrove left a comment

andygrove commented Sep 7, 2022

avantgardnerio left a comment

avantgardnerio Sep 8, 2022

comphead Sep 8, 2022

alamb Sep 8, 2022

comphead commented Sep 8, 2022

alamb left a comment

ursabot commented Sep 8, 2022

Review use of panic in datafusion-proto crate #3365

Review use of panic in datafusion-proto crate #3365

Conversation

comphead commented Sep 5, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

codecov-commenter commented Sep 5, 2022 • edited Loading

Codecov Report

alamb commented Sep 6, 2022

andygrove Sep 6, 2022

Choose a reason for hiding this comment

comphead Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

andygrove commented Sep 6, 2022

comphead commented Sep 7, 2022

alamb commented Sep 7, 2022

comphead commented Sep 7, 2022

comphead commented Sep 7, 2022

andygrove left a comment

Choose a reason for hiding this comment

andygrove commented Sep 7, 2022

avantgardnerio left a comment

Choose a reason for hiding this comment

avantgardnerio Sep 8, 2022

Choose a reason for hiding this comment

comphead Sep 8, 2022

Choose a reason for hiding this comment

alamb Sep 8, 2022

Choose a reason for hiding this comment

comphead commented Sep 8, 2022

alamb left a comment

Choose a reason for hiding this comment

ursabot commented Sep 8, 2022

codecov-commenter commented Sep 5, 2022 •

edited

Loading

comphead Sep 8, 2022 •

edited

Loading