Skip to content

GH-50294: [C++][R] Missing typename in ulp_distance.cc breaks clang-15 builds#50296

Merged
thisisnic merged 1 commit into
apache:mainfrom
thisisnic:GH-50294-typename
Jun 30, 2026
Merged

GH-50294: [C++][R] Missing typename in ulp_distance.cc breaks clang-15 builds#50296
thisisnic merged 1 commit into
apache:mainfrom
thisisnic:GH-50294-typename

Conversation

@thisisnic

@thisisnic thisisnic commented Jun 30, 2026

Copy link
Copy Markdown
Member

Rationale for this change

Broken nightlies due to older clang version on CRAN

What changes are included in this PR?

Add typename explicitly

Are these changes tested?

Will run CI

Are there any user-facing changes?

No

Copilot AI review requested due to automatic review settings June 30, 2026 07:57
@thisisnic thisisnic force-pushed the GH-50294-typename branch from d504ea3 to c725a67 Compare June 30, 2026 07:57
@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50294 has been automatically assigned in GitHub to PR creator.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50294 has no components, please add labels for components.

@thisisnic

Copy link
Copy Markdown
Member Author

@github-actions crossbow submit test-r-linux-as-cran

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a C++ template-dependent type issue that breaks older clang builds, and also expands Arrow R/Python support for view-based string/binary types to improve round-tripping and dictionary handling.

Changes:

  • Fix clang-15 build failure by adding missing typename to a dependent type in ulp_distance.cc.
  • Add R type bindings + conversion support for string_view / binary_view, including dictionary round-trips.
  • Update Python dictionary conversion for string-view-like values and add/extend tests/docs/exports for the new R types.

Reviewed changes

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
r/tests/testthat/test-Table.R Adds Table-level round-trip tests for string_view and binary_view (incl. dictionary).
r/tests/testthat/test-data-type.R Adds datatype behavior tests for string_view and binary_view; includes code roundtrip coverage.
r/tests/testthat/test-Array.R Adds Array round-trip tests for string_view and binary_view (incl. long values).
r/src/r_to_arrow.cpp Extends R→Arrow conversion to handle StringView and BinaryView-like builders safely.
r/src/datatype.cpp Adds R6 class mapping + exported initializers for utf8_view / binary_view.
r/src/arrowExports.cpp Registers new exported C++ entry points for the added R datatypes.
r/src/array_to_vector.cpp Extends Arrow→R conversion to support STRING_VIEW and BINARY_VIEW arrays and dictionary level coercion.
r/R/type.R Adds utf8_view() / string_view() and binary_view() to the R API + canonical type string mapping.
r/R/arrowExports.R Adds R .Call() wrappers for the new datatype initializers.
r/NAMESPACE Exports utf8_view, string_view, and binary_view.
r/man/data-type.Rd Documents the new datatype helpers.
python/pyarrow/src/arrow/python/python_to_arrow.cc Adjusts dictionary conversion to append via std::string_view for string-view-like values.
cpp/src/arrow/util/ulp_distance.cc Adds missing typename for clang compatibility.
cpp/src/arrow/util/converter.h Enables dictionary conversion support for StringViewType / BinaryViewType.

Comment thread cpp/src/arrow/util/ulp_distance.cc
@github-actions

Copy link
Copy Markdown

Revision: 98da882

Submitted crossbow builds: ursacomputing/crossbow @ actions-505abd297a

Task Status
test-r-linux-as-cran GitHub Actions

Copilot AI review requested due to automatic review settings June 30, 2026 08:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@thisisnic thisisnic marked this pull request as draft June 30, 2026 08:05

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you marked it as draft, but LGTM anyway :)

@thisisnic

thisisnic commented Jun 30, 2026

Copy link
Copy Markdown
Member Author

Ah sorry, I marked it as draft as I'd made a mistake and the issue is our CI is out date so the fix might not be needed to the code but to the CI instead. But I guess the fix isn't wrong, just now unnecessary? Looks like it's good practice so I'll just open a new issue for the CI update and merge this.

@thisisnic thisisnic marked this pull request as ready for review June 30, 2026 08:18
@thisisnic

Copy link
Copy Markdown
Member Author

CI failure unrelated so I'll merge

@thisisnic thisisnic merged commit 681ef42 into apache:main Jun 30, 2026
62 of 63 checks passed
@thisisnic thisisnic removed the awaiting committer review Awaiting committer review label Jun 30, 2026
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 681ef42.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants