You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "CrystalZhou0529 (via GitHub)" <gi...@apache.org> on 2024/09/03 18:26:00 UTC

[PR] GH-41365: [C++] Enhance error message for URI parsing [arrow]

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

   <!--
   Thanks for opening a pull request!
   If this is your first pull request you can find detailed information on how 
   to contribute here:
     * [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
     * [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
   
   
   If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
   
   Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
   
   Then could you also rename the pull request title in the following format?
   
       GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
   or
   
       MINOR: [${COMPONENT}] ${SUMMARY}
   
   In the case of PARQUET issues on JIRA the title also supports:
   
       PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
   
   -->
   
   ### Rationale for this change
   Issue: #41365 
   
   We want to enhance error message for URI parsing error to provide more information for the syntax error scenario.
   
   When error message is generated from `uriParseSingleUriExA`, the return value might indicate a `URI_ERROR_SYNTAX` error, and `error_pos` would be set to the position causing syntax error. ([uriparser/Uri.h](https://github.com/apache/arrow/blob/c455d6b8c4ae2cb22baceb4c27e1325b973d39e1/cpp/src/arrow/vendored/uriparser/Uri.h#L288))
   
   In the new error message, it includes the character causing syntax error and its position, so users can have a better idea why the error happens.
   
   ### What changes are included in this PR?
   
   - Error message change in URI parsing function.
   
   <!--
   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?
   
   PR includes unit tests.
   
   ### Are there any user-facing changes?
   
   Yes, but only for error message.
   


-- 
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-43967: [C++] Enhance error message for URI parsing [arrow]

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


-- 
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-41365: [C++] Enhance error message for URI parsing [arrow]

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

   @AlenkaF yes this is already tracked and I checked the CIs too. And not relevant to 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

Re: [PR] GH-43967: [C++] Enhance error message for URI parsing [arrow]

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

   Thanks @CrystalZhou0529!


-- 
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-41365: [C++] Enhance error message for URI parsing [arrow]

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

   :warning: GitHub issue #41365 **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-41365: [C++] Enhance error message for URI parsing [arrow]

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

   @AlenkaF 
   
   > First: could we also add a test on the Python side to check it works correctly there also?
   
   I think the problem is that PyArrow swallows the URI error and falls back to the local filesystem (which is often legitimate, but probably not if the user really meant to pass a S3 URI :-)).


-- 
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-41365: [C++] Enhance error message for URI parsing [arrow]

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

   > The Java failure is something I noticed on other PRs also if I am not mistaken?
   
   Yes, it's quite recurring. I'm not sure there's an issue open for it?


-- 
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-43967: [C++] Enhance error message for URI parsing [arrow]

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

   :warning: GitHub issue #43967 **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-43967: [C++] Enhance error message for URI parsing [arrow]

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

   After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 9336bbeaa76348367c6a8dd3048088dae203b836.
   
   There were no benchmark performance regressions. 🎉
   
   The [full Conbench report](https://github.com/apache/arrow/runs/29740431958) has more details. It also includes information about 22 possible false positives for unstable benchmarks that are known to sometimes produce them.


-- 
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-41365: [C++] Enhance error message for URI parsing [arrow]

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

   I think this might be the open issue connected to the Java failures: https://github.com/apache/arrow/issues/43576
   
   Will merge this PR as the CI is green.


-- 
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-41365: [C++] Enhance error message for URI parsing [arrow]

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


##########
cpp/src/arrow/filesystem/filesystem_test.cc:
##########
@@ -632,6 +633,13 @@ TEST_F(TestMockFS, FileSystemFromUri) {
   ASSERT_OK_AND_ASSIGN(fs_, FileSystemFromUri("mock:///foo/bar?q=zzz", &path));
   ASSERT_EQ(path, "foo/bar");
   CheckDirs({});
+  ASSERT_OK_AND_ASSIGN(fs_, FileSystemFromUri("mock:/folder+name/bar?q=zzz", &path));

Review Comment:
   Well, there _is_ a test with whitespace below :-)



-- 
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-41365: [C++] Enhance error message for URI parsing [arrow]

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


##########
cpp/src/arrow/filesystem/filesystem_test.cc:
##########
@@ -632,6 +633,13 @@ TEST_F(TestMockFS, FileSystemFromUri) {
   ASSERT_OK_AND_ASSIGN(fs_, FileSystemFromUri("mock:///foo/bar?q=zzz", &path));
   ASSERT_EQ(path, "foo/bar");
   CheckDirs({});
+  ASSERT_OK_AND_ASSIGN(fs_, FileSystemFromUri("mock:/folder+name/bar?q=zzz", &path));

Review Comment:
   Should `+` in the path raise or is that expected for white space? In any case we should add URIs with white space to the test as this was reported in the issue.



-- 
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-43967: [C++] Enhance error message for URI parsing [arrow]

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

   :warning: GitHub issue #43967 **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-43967: [C++] Enhance error message for URI parsing [arrow]

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

   > > First: could we also add a test on the Python side to check it works correctly there also?
   > 
   > I think the problem is that PyArrow swallows the URI error and falls back to the local filesystem (which is often legitimate, but probably not if the user really meant to pass a S3 URI :-)).
   
   Given that this PR is not solving the original issue (I _do_ think we can actually fix that by being more strict in when we swallow that error, and actually bubble it up to the user more often), I opened a separate issue focused on improving the C++ error message and linked this PR to that: https://github.com/apache/arrow/issues/43967


-- 
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-41365: [C++] Enhance error message for URI parsing [arrow]

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

   > I think the problem is that PyArrow swallows the URI error and falls back to the local filesystem (which is often legitimate, but probably not if the user really meant to pass a S3 URI :-)).
   
   Yeah, the content in the issue should have given me this explanation already. Thanks again ;)
   
   The Java failure is something I noticed on other PRs also if I am not mistaken?


-- 
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-41365: [C++] Enhance error message for URI parsing [arrow]

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


##########
cpp/src/arrow/filesystem/filesystem_test.cc:
##########
@@ -632,6 +633,13 @@ TEST_F(TestMockFS, FileSystemFromUri) {
   ASSERT_OK_AND_ASSIGN(fs_, FileSystemFromUri("mock:///foo/bar?q=zzz", &path));
   ASSERT_EQ(path, "foo/bar");
   CheckDirs({});
+  ASSERT_OK_AND_ASSIGN(fs_, FileSystemFromUri("mock:/folder+name/bar?q=zzz", &path));

Review Comment:
   Oh 🤦‍♀️  sorry about that :D



-- 
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-43967: [C++] Enhance error message for URI parsing [arrow]

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

   :warning: GitHub issue #43967 **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