You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "felipecrv (via GitHub)" <gi...@apache.org> on 2023/12/20 15:28:12 UTC

[PR] GH-39322: [C++] Forward arguments to ExceptionToStatus all the way to Status::FromArgs [arrow]

felipecrv opened a new pull request, #39323:
URL: https://github.com/apache/arrow/pull/39323

   ### Rationale for this change
   
   This simplifies the creation of long error messages and leads to the use of a string builder to construct the error message.
   
   ### What changes are included in this PR?
   
    - std::forward in ExceptionToStatus
    - A few nitpicky changes
    - Simplification of the error message text
    - Moving the signature of `CheckIfHierarchicalNamespaceIsEnabled` to `azurefs_internal.h` to reduce the size of `azurefs.h` -- implementation remains in `azurefs.cc`
   
   ### Are these changes tested?
   
   Yes. By existing 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.

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

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

Re: [PR] GH-39322: [C++] Forward arguments to ExceptionToStatus all the way to Status::FromArgs [arrow]

Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #39323:
URL: https://github.com/apache/arrow/pull/39323#issuecomment-1864673812

   @Tom-Newton @kou 


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

Re: [PR] GH-39322: [C++] Forward arguments to ExceptionToStatus all the way to Status::FromArgs [arrow]

Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #39323:
URL: https://github.com/apache/arrow/pull/39323#discussion_r1432948156


##########
cpp/src/arrow/filesystem/azurefs.h:
##########
@@ -21,6 +21,7 @@
 #include <string>
 #include <vector>
 
+#include "arrow/filesystem/azurefs_internal.h"

Review Comment:
   It's a mistake! Fixing.



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

Re: [PR] GH-39322: [C++] Forward arguments to ExceptionToStatus all the way to Status::FromArgs [arrow]

Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #39323:
URL: https://github.com/apache/arrow/pull/39323#discussion_r1432949774


##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -571,15 +581,14 @@ class ObjectInputFile final : public io::RandomAccessFile {
   std::shared_ptr<const KeyValueMetadata> metadata_;
 };
 
-Status CreateEmptyBlockBlob(std::shared_ptr<Blobs::BlockBlobClient> block_blob_client) {
+Status CreateEmptyBlockBlob(Blobs::BlockBlobClient& block_blob_client) {

Review Comment:
   I can make it const. Pushing a fix.



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

Re: [PR] GH-39322: [C++] Forward arguments to ExceptionToStatus all the way to Status::FromArgs [arrow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39323:
URL: https://github.com/apache/arrow/pull/39323#issuecomment-1864674124

   :warning: GitHub issue #39322 **has been automatically assigned in GitHub** to PR creator.


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

Re: [PR] GH-39322: [C++] Forward arguments to ExceptionToStatus all the way to Status::FromArgs [arrow]

Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #39323:
URL: https://github.com/apache/arrow/pull/39323#issuecomment-1864818782

   It seems you need to fix or circumvent the linting failure.


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

Re: [PR] GH-39322: [C++] Forward arguments to ExceptionToStatus all the way to Status::FromArgs [arrow]

Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv merged PR #39323:
URL: https://github.com/apache/arrow/pull/39323


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

Re: [PR] GH-39322: [C++] Forward arguments to ExceptionToStatus all the way to Status::FromArgs [arrow]

Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39323:
URL: https://github.com/apache/arrow/pull/39323#issuecomment-1865823879

   After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 91b2243e2753bb1a4ccd645dd41d74b1d0b077c0.
   
   There were no benchmark performance regressions. 🎉
   
   The [full Conbench report](https://github.com/apache/arrow/runs/19852574497) has more details.


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

Re: [PR] GH-39322: [C++] Forward arguments to ExceptionToStatus all the way to Status::FromArgs [arrow]

Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39323:
URL: https://github.com/apache/arrow/pull/39323#discussion_r1432871317


##########
cpp/src/arrow/filesystem/azurefs.h:
##########
@@ -21,6 +21,7 @@
 #include <string>
 #include <vector>
 
+#include "arrow/filesystem/azurefs_internal.h"

Review Comment:
   Is this a mistake? We should not include an internal header in a public header.



##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -571,15 +581,14 @@ class ObjectInputFile final : public io::RandomAccessFile {
   std::shared_ptr<const KeyValueMetadata> metadata_;
 };
 
-Status CreateEmptyBlockBlob(std::shared_ptr<Blobs::BlockBlobClient> block_blob_client) {
+Status CreateEmptyBlockBlob(Blobs::BlockBlobClient& block_blob_client) {

Review Comment:
   If not const, can we pass it as pointer?
   ```suggestion
   Status CreateEmptyBlockBlob(Blobs::BlockBlobClient* block_blob_client) {
   ```



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

Re: [PR] GH-39322: [C++] Forward arguments to ExceptionToStatus all the way to Status::FromArgs [arrow]

Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #39323:
URL: https://github.com/apache/arrow/pull/39323#issuecomment-1864877997

   > It seems you need to fix or circumvent the linting failure.
   
   Trying something! 🤞 


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