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 2023/01/19 12:35:45 UTC

[GitHub] [arrow-datafusion] DDtKey opened a new pull request, #4983: fix(4981): incorrect error wrapping in `OnceFut`

DDtKey opened a new pull request, #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983

   # Which issue does this PR close?
   
   
   Closes #4981
   
   # Rationale for this change
   
   <!--
    Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
    Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.  
   -->
   
   # What changes are included in this PR?
   
   <!--
   There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
   -->
   
   # Are these changes tested?
   
   I've added test which failed before these changes. 
   
   # Are there any user-facing changes?
   
   <!--
   If there are user-facing changes then we may require documentation to be updated before approving the PR.
   -->
   
   <!--
   If there are any breaking changes to public APIs, please add the `api change` label.
   -->


-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] DDtKey commented on pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
DDtKey commented on PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#issuecomment-1397575303

   > @DDtKey does https://github.com/apache/arrow-datafusion/pull/4992 accomplish the same desired outcome?
   
   Don't think so, it was literally string instead of error. Take a look at code before these changes. It had `e.to_string()`


-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] DDtKey commented on a diff in pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
DDtKey commented on code in PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#discussion_r1081937386


##########
datafusion/core/src/physical_plan/joins/utils.rs:
##########
@@ -438,7 +438,7 @@ impl<T: 'static> OnceAsync<T> {
 }
 
 /// The shared future type used internally within [`OnceAsync`]
-type OnceFutPending<T> = Shared<BoxFuture<'static, Arc<Result<T>>>>;
+type OnceFutPending<T> = Shared<BoxFuture<'static, Arc<SharedResult<T>>>>;

Review Comment:
   `OnceFut` is cloneable, while `SharedResult` wraps only `Err` into `Arc` to allow cloning the error itself 🤔 
   
   I decided to declare it as `SharedResult<Arc<T>>` instead, so it won't be `Arc` inside `Arc`. It will be either content of `Ok` or `Error` wrapped into `Arc`.



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#discussion_r1082358899


##########
datafusion/common/src/error.rs:
##########
@@ -36,6 +36,9 @@ use sqlparser::parser::ParserError;
 /// Result type for operations that could result in an [DataFusionError]
 pub type Result<T> = result::Result<T, DataFusionError>;
 
+/// Result type for operations that could result in an [DataFusionError] and needs to be shared (wrapped into `Arc`).
+pub type SharedResult<T> = result::Result<T, Arc<DataFusionError>>;

Review Comment:
   > allow clonning errors in datafustion & arrow.
   
   I don't see any other examples of this pattern?



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] alamb merged pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
alamb merged PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983


-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] ursabot commented on pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#issuecomment-1398225867

   Benchmark runs are scheduled for baseline = 6dce728a3c7130ca3590a16f413c7c6ccb7209b7 and contender = 22d106a6564345a746699cd5eb1fc84b9267ce83. 22d106a6564345a746699cd5eb1fc84b9267ce83 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
   Conbench compare runs links:
   [Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/058eb473c45c46b99d8a8b5a5425beaa...044241bbf7bd43bbab0fd1d22d9ff5bf/)
   [Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/38bff27cf5664f88a701112521defe1a...f58ae74daf734067afb360384290f899/)
   [Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/09a3e8b9632e4a078b5ff2115a7b7c99...51ab53df86d64cc08a9bdb0f318de977/)
   [Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a600644eb4684ef9ba41aa02c6d551e4...70b1345bb0a74810b6c5d6f439ad7ac2/)
   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
   


-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#discussion_r1082365743


##########
datafusion/common/src/error.rs:
##########
@@ -36,6 +36,9 @@ use sqlparser::parser::ParserError;
 /// Result type for operations that could result in an [DataFusionError]
 pub type Result<T> = result::Result<T, DataFusionError>;
 
+/// Result type for operations that could result in an [DataFusionError] and needs to be shared (wrapped into `Arc`).
+pub type SharedResult<T> = result::Result<T, Arc<DataFusionError>>;

Review Comment:
   See https://github.com/apache/arrow-datafusion/pull/4992 for the code (that does indeed walk over Arcs). It was non ideal however. Figuring out some way to avoid Arc'ing errors would be better in my opinion (not for this PR)



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] avantgardnerio commented on pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#issuecomment-1397568671

   @DDtKey does https://github.com/apache/arrow-datafusion/pull/4992 accomplish the same desired outcome?


-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] DDtKey commented on a diff in pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
DDtKey commented on code in PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#discussion_r1082346749


##########
datafusion/common/src/error.rs:
##########
@@ -36,6 +36,9 @@ use sqlparser::parser::ParserError;
 /// Result type for operations that could result in an [DataFusionError]
 pub type Result<T> = result::Result<T, DataFusionError>;
 
+/// Result type for operations that could result in an [DataFusionError] and needs to be shared (wrapped into `Arc`).
+pub type SharedResult<T> = result::Result<T, Arc<DataFusionError>>;

Review Comment:
   Well, I observed that `Result<T, Arc<Error>` is quite common approach to allow clonning errors in datafustion & arrow.
   
   And not to miss such kind of errors, I think it would be nice to have it as general type. 
   Take a look at #4992 and even current `find_root` impl, it's easy to forget that error could be wrapped into `Arc` actually (IMO)
   
   I believe this type should be used when it's needed to explicitly declare that error is wrapped into `Arc`. And could be used in other places later.
   
   And this PR doesn't return this type, however it will be inside `ArrowError::ExternalError(..)`, so actually it's way of explicit usage to be able to check all usages of this pattern



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#discussion_r1082349004


##########
datafusion/core/src/physical_plan/joins/utils.rs:
##########
@@ -691,8 +695,8 @@ impl<T: 'static> OnceFut<T> {
             OnceFutState::Pending(_) => unreachable!(),
             OnceFutState::Ready(r) => Poll::Ready(
                 r.as_ref()
-                    .as_ref()
-                    .map_err(|e| ArrowError::ExternalError(e.to_string().into())),
+                    .map(|r| r.as_ref())
+                    .map_err(|e| ArrowError::ExternalError(Box::new(e.clone()))),

Review Comment:
   ```suggestion
                       .map_err(|e| ArrowError::ExternalError(Box::new(e.clone()))),
   ```



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] avantgardnerio commented on a diff in pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#discussion_r1081861681


##########
datafusion/core/src/physical_plan/joins/utils.rs:
##########
@@ -438,7 +438,7 @@ impl<T: 'static> OnceAsync<T> {
 }
 
 /// The shared future type used internally within [`OnceAsync`]
-type OnceFutPending<T> = Shared<BoxFuture<'static, Arc<Result<T>>>>;
+type OnceFutPending<T> = Shared<BoxFuture<'static, Arc<SharedResult<T>>>>;

Review Comment:
   Now that `SharedResult` has an `Arc`, can we avoid the double-arc here?



##########
datafusion/core/src/physical_plan/joins/utils.rs:
##########
@@ -653,7 +653,7 @@ fn get_int_range(min: ScalarValue, max: ScalarValue) -> Option<usize> {
 
 enum OnceFutState<T> {
     Pending(OnceFutPending<T>),
-    Ready(Arc<Result<T>>),
+    Ready(Arc<SharedResult<T>>),

Review Comment:
   Same



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] DDtKey commented on a diff in pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
DDtKey commented on code in PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#discussion_r1081937386


##########
datafusion/core/src/physical_plan/joins/utils.rs:
##########
@@ -438,7 +438,7 @@ impl<T: 'static> OnceAsync<T> {
 }
 
 /// The shared future type used internally within [`OnceAsync`]
-type OnceFutPending<T> = Shared<BoxFuture<'static, Arc<Result<T>>>>;
+type OnceFutPending<T> = Shared<BoxFuture<'static, Arc<SharedResult<T>>>>;

Review Comment:
   `OnceFut` is cloneable, while `SharedResult` wraps only `Err` into `Arc` to allow cloning the error itself 🤔 
   
   I decided to declare it as `SharedResult<Arc<T>>` instead, so it won't be `Arc` inside `Arc`. It will be either result or error wrapped into `Arc`.



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#discussion_r1082338149


##########
datafusion/common/src/error.rs:
##########
@@ -36,6 +36,9 @@ use sqlparser::parser::ParserError;
 /// Result type for operations that could result in an [DataFusionError]
 pub type Result<T> = result::Result<T, DataFusionError>;
 
+/// Result type for operations that could result in an [DataFusionError] and needs to be shared (wrapped into `Arc`).
+pub type SharedResult<T> = result::Result<T, Arc<DataFusionError>>;

Review Comment:
   Given this PR never actually returns this type, do we need to declare it 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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] alamb commented on pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#issuecomment-1398224025

   Thanks @DDtKey 


-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#discussion_r1082358899


##########
datafusion/common/src/error.rs:
##########
@@ -36,6 +36,9 @@ use sqlparser::parser::ParserError;
 /// Result type for operations that could result in an [DataFusionError]
 pub type Result<T> = result::Result<T, DataFusionError>;
 
+/// Result type for operations that could result in an [DataFusionError] and needs to be shared (wrapped into `Arc`).
+pub type SharedResult<T> = result::Result<T, Arc<DataFusionError>>;

Review Comment:
   > allow clonning errors in datafustion & arrow.
   
   I don't see any other examples of this pattern?
   
   > so actually it's way of explicit usage to be able to check all usages of this pattern
   
   I'd hope that any codepath that cares about these variants would walk the `source` tree and effectively blindly skip over the Arc...



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] DDtKey commented on a diff in pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
DDtKey commented on code in PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#discussion_r1081937386


##########
datafusion/core/src/physical_plan/joins/utils.rs:
##########
@@ -438,7 +438,7 @@ impl<T: 'static> OnceAsync<T> {
 }
 
 /// The shared future type used internally within [`OnceAsync`]
-type OnceFutPending<T> = Shared<BoxFuture<'static, Arc<Result<T>>>>;
+type OnceFutPending<T> = Shared<BoxFuture<'static, Arc<SharedResult<T>>>>;

Review Comment:
   `OnceFut` is cloneable, while `SharedResult` wraps only `Err` into `Arc` to allow cloning the error itself 🤔 
   
   I decided to declare it as `SharedResult<Arc<T>>` instead, so it won't be `Arc` inside `Arc`. It will be either result or error wrapped into `Arc`.
   
   Take a look at the last commit.



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] DDtKey commented on a diff in pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
DDtKey commented on code in PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#discussion_r1082346749


##########
datafusion/common/src/error.rs:
##########
@@ -36,6 +36,9 @@ use sqlparser::parser::ParserError;
 /// Result type for operations that could result in an [DataFusionError]
 pub type Result<T> = result::Result<T, DataFusionError>;
 
+/// Result type for operations that could result in an [DataFusionError] and needs to be shared (wrapped into `Arc`).
+pub type SharedResult<T> = result::Result<T, Arc<DataFusionError>>;

Review Comment:
   Well, I observed that `Result<T, Arc<Error>` is quite common approach to allow clonning errors in datafustion & arrow.
   
   And not to miss such kind of errors, I think it would be nice to have it as general type. 
   Take a look at #4992 and even current `find_root` impl, it's easy to forget that error could be wrapped into `Arc` actually (IMO)
   
   I believe this type should be used when it's needed to explicitly declare that error is wrapped into `Arc`. And could be used in other places later.



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#discussion_r1082338149


##########
datafusion/common/src/error.rs:
##########
@@ -36,6 +36,9 @@ use sqlparser::parser::ParserError;
 /// Result type for operations that could result in an [DataFusionError]
 pub type Result<T> = result::Result<T, DataFusionError>;
 
+/// Result type for operations that could result in an [DataFusionError] and needs to be shared (wrapped into `Arc`).
+pub type SharedResult<T> = result::Result<T, Arc<DataFusionError>>;

Review Comment:
   Given this PR never actually returns this type, do we need to declare it here? Or could we just move it into utils?



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4983: fix(4981): incorrect error wrapping in `OnceFut`

Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #4983:
URL: https://github.com/apache/arrow-datafusion/pull/4983#discussion_r1082340631


##########
datafusion/core/src/physical_plan/joins/utils.rs:
##########
@@ -653,7 +653,7 @@ fn get_int_range(min: ScalarValue, max: ScalarValue) -> Option<usize> {
 
 enum OnceFutState<T> {
     Pending(OnceFutPending<T>),
-    Ready(Arc<Result<T>>),
+    Ready(SharedResult<Arc<T>>),

Review Comment:
   ```suggestion
       Ready(Arc<Result<T>>),
   ```
   Perhaps?



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org