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/10/13 21:33:19 UTC
[PR] GH-38263 [C++]: Use re2::StringPiece instead of std::string_view when using re2 [arrow]
felipecrv opened a new pull request, #38265:
URL: https://github.com/apache/arrow/pull/38265
### Rationale for this change
The MSVC compiler doesn't seem to allow user code to assume `std::string_view::const_iterator` is `const char*`, so using only `re2::StringPiece` and preferring to call `.data()` instead of `.begin()` should make things more uniform across different compilers and STL implementations.
### What changes are included in this PR?
- Using `re2::StringPiece` instead of `std::string_view` to interact with `re2`
- Use `data()` instead of `begin()` where a `char*` is expected
### 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-38263 [C++]: Prefer to call string_view::data() instead of begin() where a char pointer is expected [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #38265:
URL: https://github.com/apache/arrow/pull/38265#issuecomment-1763332622
> This should IMO not miss the 14.0 release
I agree, I'll add it to 14.0.0
--
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-38263 [C++]: Prefer to call string_view::data() instead of begin() where a char pointer is expected [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #38265:
URL: https://github.com/apache/arrow/pull/38265#issuecomment-1767413558
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 93ca3b2fdfb813542b14759c4d918553500bc307.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/17801857792) has more details. It also includes information about 3 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-38263 [C++]: Prefer to call string_view::data() instead of begin() where a char pointer is expected [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #38265:
URL: https://github.com/apache/arrow/pull/38265#issuecomment-1764440764
@felipecrv I am validating but this seems to make the `test-build-vcpkg-win` build to fail on the maintenance branch:
```
D:/a/crossbow/crossbow/arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc(1970,1): error C2664: 'arrow::Status arrow::compute::internal::`anonymous-namespace'::RegexSubstringReplacer<arrow::LargeStringType>::ReplaceString(re2::StringPiece,arrow::TypedBufferBuilder<uint8_t,void> *) const': cannot convert argument 1 from 'std::string_view' to 're2::StringPiece' (compiling source file D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_21_cxx.cxx) [D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\arrow_shared.vcxproj]
D:/a/crossbow/crossbow/arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc(1970,1): message : No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called (compiling source file D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_21_cxx.cxx) [D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\arrow_shared.vcxproj]
D:/a/crossbow/crossbow/arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc(2066,10): message : see declaration of 'arrow::compute::internal::`anonymous-namespace'::RegexSubstringReplacer<arrow::LargeStringType>::ReplaceString' (compiling source file D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_21_cxx.cxx) [D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\arrow_shared.vcxproj]
D:/a/crossbow/crossbow/arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc(1962): message : while compiling class template member function 'arrow::Status arrow::compute::internal::`anonymous-namespace'::ReplaceSubstring<arrow::LargeStringType,arrow::compute::internal::`anonymous-namespace'::RegexSubstringReplacer<arrow::LargeStringType>>::Replace(arrow::compute::KernelContext *,const arrow::compute::ExecSpan &,const Replacer &,arrow::compute::ExecResult *)' [D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\arrow_shared.vcxproj]
with
[
Replacer=arrow::compute::internal::`anonymous-namespace'::RegexSubstringReplacer<arrow::LargeStringType>
] (compiling source file D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_21_cxx.cxx)
D:/a/crossbow/crossbow/arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc(1958): message : see reference to function template instantiation 'arrow::Status arrow::compute::internal::`anonymous-namespace'::ReplaceSubstring<arrow::LargeStringType,arrow::compute::internal::`anonymous-namespace'::RegexSubstringReplacer<arrow::LargeStringType>>::Replace(arrow::compute::KernelContext *,const arrow::compute::ExecSpan &,const Replacer &,arrow::compute::ExecResult *)' being compiled [D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\arrow_shared.vcxproj]
with
[
Replacer=arrow::compute::internal::`anonymous-namespace'::RegexSubstringReplacer<arrow::LargeStringType>
] (compiling source file D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_21_cxx.cxx)
D:\a\crossbow\crossbow\arrow\cpp\src\arrow/compute/kernels/codegen_internal.h(1249): message : see reference to class template instantiation 'arrow::compute::internal::`anonymous-namespace'::ReplaceSubstring<arrow::LargeStringType,arrow::compute::internal::`anonymous-namespace'::RegexSubstringReplacer<arrow::LargeStringType>>' being compiled (compiling source file D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_21_cxx.cxx) [D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\arrow_shared.vcxproj]
D:/a/crossbow/crossbow/arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc(2158): message : see reference to function template instantiation 'arrow::compute::ArrayKernelExec (__cdecl *arrow::compute::internal::GenerateVarBinaryToVarBinary<arrow::compute::internal::`anonymous-namespace'::ReplaceSubstringRegex,>(arrow::compute::internal::detail::GetTypeId))(arrow::compute::KernelContext *,const arrow::compute::ExecSpan &,arrow::compute::ExecResult *)' being compiled (compiling source file D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_21_cxx.cxx) [D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\arrow_shared.vcxproj]
```
See: https://github.com/ursacomputing/crossbow/actions/runs/6533642461/job/17739225267
--
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-38263 [C++]: Prefer to call string_view::data() instead of begin() where a char pointer is expected [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38265:
URL: https://github.com/apache/arrow/pull/38265#issuecomment-1764447693
Revision: 63c011c93e905029d3d0c0dc7a5b9661f79e7ab3
Submitted crossbow builds: [ursacomputing/crossbow @ actions-4d249f0f30](https://github.com/ursacomputing/crossbow/branches/all?query=actions-4d249f0f30)
|Task|Status|
|----|------|
|test-build-vcpkg-win|[](https://github.com/ursacomputing/crossbow/actions/runs/6534019769/job/17740342816)|
--
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-38263 [C++]: Prefer to call string_view::data() instead of begin() where a char pointer is expected [arrow]
Posted by "h-vetinari (via GitHub)" <gi...@apache.org>.
h-vetinari commented on PR #38265:
URL: https://github.com/apache/arrow/pull/38265#issuecomment-1762887763
Thanks a lot! I've tested the changes here on top of 13.0.0 in https://github.com/conda-forge/arrow-cpp-feedstock/pull/1200, and it compiles and passes the 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-38263 [C++]: Prefer to call string_view::data() instead of begin() where a char pointer is expected [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd merged PR #38265:
URL: https://github.com/apache/arrow/pull/38265
--
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-38263 [C++]: Use re2::StringPiece instead of std::string_view when using re2 [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38265:
URL: https://github.com/apache/arrow/pull/38265#issuecomment-1762251458
:warning: GitHub issue #38263 **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-38263 [C++]: Prefer to call string_view::data() instead of begin() where a char pointer is expected [arrow]
Posted by "h-vetinari (via GitHub)" <gi...@apache.org>.
h-vetinari commented on PR #38265:
URL: https://github.com/apache/arrow/pull/38265#issuecomment-1763314780
FYI @jorisvandenbossche @raulcd
This should IMO not miss the 14.0 release; risk should be minimal (for reference, I backported it all the way back to 10.x on the conda-forge feedstock without issues), and it's necessary to handle newer re2 versions like we are using in conda-forge now.
--
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-38263 [C++]: Prefer to call string_view::data() instead of begin() where a char pointer is expected [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #38265:
URL: https://github.com/apache/arrow/pull/38265#issuecomment-1764505871
> @felipecrv I am validating but this seems to make the `test-build-vcpkg-win` build to fail on the maintenance branch:
>
> ```
> D:/a/crossbow/crossbow/arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc(1970,1): error C2664: 'arrow::Status arrow::compute::internal::`anonymous-namespace'::RegexSubstringReplacer<arrow::LargeStringType>::ReplaceString(re2::StringPiece,arrow::TypedBufferBuilder<uint8_t,void> *) const': cannot convert argument 1 from 'std::string_view' to 're2::StringPiece' (compiling source file D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_21_cxx.cxx) [D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\arrow_shared.vcxproj]
> D:/a/crossbow/crossbow/arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc(1970,1): message : No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called (compiling source file D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_21_cxx.cxx) [D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\arrow_shared.vcxproj]
> D:/a/crossbow/crossbow/arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc(2066,10): message : see declaration of 'arrow::compute::internal::`anonymous-namespace'::RegexSubstringReplacer<arrow::LargeStringType>::ReplaceString' (compiling source file D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_21_cxx.cxx) [D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\arrow_shared.vcxproj]
> D:/a/crossbow/crossbow/arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc(1962): message : while compiling class template member function 'arrow::Status arrow::compute::internal::`anonymous-namespace'::ReplaceSubstring<arrow::LargeStringType,arrow::compute::internal::`anonymous-namespace'::RegexSubstringReplacer<arrow::LargeStringType>>::Replace(arrow::compute::KernelContext *,const arrow::compute::ExecSpan &,const Replacer &,arrow::compute::ExecResult *)' [D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\arrow_shared.vcxproj]
> with
> [
> Replacer=arrow::compute::internal::`anonymous-namespace'::RegexSubstringReplacer<arrow::LargeStringType>
> ] (compiling source file D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_21_cxx.cxx)
> D:/a/crossbow/crossbow/arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc(1958): message : see reference to function template instantiation 'arrow::Status arrow::compute::internal::`anonymous-namespace'::ReplaceSubstring<arrow::LargeStringType,arrow::compute::internal::`anonymous-namespace'::RegexSubstringReplacer<arrow::LargeStringType>>::Replace(arrow::compute::KernelContext *,const arrow::compute::ExecSpan &,const Replacer &,arrow::compute::ExecResult *)' being compiled [D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\arrow_shared.vcxproj]
> with
> [
> Replacer=arrow::compute::internal::`anonymous-namespace'::RegexSubstringReplacer<arrow::LargeStringType>
> ] (compiling source file D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_21_cxx.cxx)
> D:\a\crossbow\crossbow\arrow\cpp\src\arrow/compute/kernels/codegen_internal.h(1249): message : see reference to class template instantiation 'arrow::compute::internal::`anonymous-namespace'::ReplaceSubstring<arrow::LargeStringType,arrow::compute::internal::`anonymous-namespace'::RegexSubstringReplacer<arrow::LargeStringType>>' being compiled (compiling source file D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_21_cxx.cxx) [D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\arrow_shared.vcxproj]
> D:/a/crossbow/crossbow/arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc(2158): message : see reference to function template instantiation 'arrow::compute::ArrayKernelExec (__cdecl *arrow::compute::internal::GenerateVarBinaryToVarBinary<arrow::compute::internal::`anonymous-namespace'::ReplaceSubstringRegex,>(arrow::compute::internal::detail::GetTypeId))(arrow::compute::KernelContext *,const arrow::compute::ExecSpan &,arrow::compute::ExecResult *)' being compiled (compiling source file D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_21_cxx.cxx) [D:\a\crossbow\crossbow\arrow\cpp\build\src\arrow\arrow_shared.vcxproj]
> ```
>
> See: https://github.com/ursacomputing/crossbow/actions/runs/6533642461/job/17739225267
Alright. I will push a fix now.
--
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-38263 [C++]: Use re2::StringPiece instead of std::string_view when using re2 [arrow]
Posted by "h-vetinari (via GitHub)" <gi...@apache.org>.
h-vetinari commented on PR #38265:
URL: https://github.com/apache/arrow/pull/38265#issuecomment-1762257686
> Using `re2::StringPiece` instead of `std::string_view` to interact with `re2`
When compiling with C++17 (as we are doing in conda-forge) those are exactly the same type. Unless arrow enforces compilation with an earlier C++ standard somewhere, I don't see how that could be 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-38263 [C++]: Use re2::StringPiece instead of std::string_view when using re2 [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #38265:
URL: https://github.com/apache/arrow/pull/38265#issuecomment-1762372014
> > Using `re2::StringPiece` instead of `std::string_view` to interact with `re2`
>
> When compiling with C++17 (as we are doing in conda-forge) those are exactly the same type. Unless arrow enforces compilation with an earlier C++ standard somewhere, I don't see how that could be the issue.
See the second commit changing from `.begin()` to `.data()`.
--
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-38263 [C++]: Prefer to call string_view::data() instead of begin() where a char pointer is expected [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #38265:
URL: https://github.com/apache/arrow/pull/38265#issuecomment-1764438621
@github-actions crossbow submit test-build-vcpkg-win
--
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