-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[R] R tests fail with "NotImplemented: construction from scalar of type numeric(0)" #37091
Comments
For more context, the body of the failing test (with extraneous tests truncated out) in R is:
Which converts an extension array to a scalar and then converts that to a string. |
I'll have a look, but also pinging @paleolimbot who knows more about our extension type implementation than I do |
This particular test was added because before #15277 a segfault occurred when attempting to print an ExtensionScalar from R. I forget the details of why the original segfault occurred, but the fix we went with is: Lines 60 to 73 in d3ccc83
@felipecrv are there some PRs I could take a look at to look for changes that may have affected this bit? |
And this is where the error is raised: arrow/cpp/src/arrow/array/util.cc Lines 802 to 804 in 1a00fec
|
Hmm...when I build Arrow off of the PR branch I get
...so I wonder if the type enum IDs changed and need to be synced with Lines 44 to 82 in 7c8f398
I'm not exactly sure how the wrong type ID would end up there, but if the wrong type ID was at Lines 60 to 63 in d3ccc83
...it would crash because casting an extension array to utf-8 is not implemented (this was the error that #15277 fixed). |
If I add |
@felipecrv - I have a solution for you in the above comment, which should fix things in your PR even if I'm not totally sure why it does. |
As this isn't a bug, I'm going to close this now |
Oh, I suspected that this could be due to R assuming a specific numbering of the @bkietz, do you know if the automatic numbering of enum entries is fully specified in C++ or is the compiler free to choose anything? |
@thisisnic @paleolimbot thanks for looking into this and sorry for the false alarm. I opened a PR that adds explicit values on the C++ side as well so no one gets confused about this again. |
@felipecrv No worries, and thanks for figuring it out for us all, as I was really confused by the reasoning there! :) |
…enum (#37149) ### Rationale for this change The enum type in C++ leaves the integer values implicitly defined and let the compiler assign the values to them automatically. This means an insertion of a new entry that is not at the end, causes implementations that rely on specific values (like R) to break with confusing error messages [1]. Assigning the values explicitly can communicate that these enum entry values are relied upon and can allow a more natural ordering of the list that is different from the numeric order the entries receive. [1] #37091 ### What changes are included in this PR? - Setting numeric values of the enum entries explicitly - Completing an equivalent enum on the R side ### Are these changes tested? N/A ### Are there any user-facing changes? No. * Closes: #37148 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>
…:type enum (apache#37149) ### Rationale for this change The enum type in C++ leaves the integer values implicitly defined and let the compiler assign the values to them automatically. This means an insertion of a new entry that is not at the end, causes implementations that rely on specific values (like R) to break with confusing error messages [1]. Assigning the values explicitly can communicate that these enum entry values are relied upon and can allow a more natural ordering of the list that is different from the numeric order the entries receive. [1] apache#37091 ### What changes are included in this PR? - Setting numeric values of the enum entries explicitly - Completing an equivalent enum on the R side ### Are these changes tested? N/A ### Are there any user-facing changes? No. * Closes: apache#37148 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>
Describe the bug, including details regarding any error messages, version, and platform.
R tests failing with
Example: https://github.com/apache/arrow/actions/runs/5803060349/job/15730480958?pr=35345
Component(s)
R
The text was updated successfully, but these errors were encountered: