Skip to content
This repository is currently being migrated. It's locked while the migration is in progress.

Fix for failing R benchmarks - cast all decimal32/64 to decimal128#139

Open
thisisnic wants to merge 8 commits intovoltrondata-labs:mainfrom
thisisnic:fix-decimal-issue
Open

Fix for failing R benchmarks - cast all decimal32/64 to decimal128#139
thisisnic wants to merge 8 commits intovoltrondata-labs:mainfrom
thisisnic:fix-decimal-issue

Conversation

@thisisnic
Copy link
Copy Markdown

@thisisnic thisisnic commented Jun 25, 2025

Benchmarks are failing (apache/arrow#46716) because decimal32 and decimal64 types have been implemented in Arrow but not Parquet, so when this type is being written to disk there is an error. This PR upcasts any decimal32/64 instances to decimal128 when writing to Parquet in R.

Haven't been able to test this out locally, so I've gone for a bit of an overkill approach and used this in calls to write_parquet().

Copy link
Copy Markdown
Contributor

@jonkeane jonkeane left a comment

Choose a reason for hiding this comment

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

Thanks for this. This looks good (though we should remove the addition in bm-write-file.R)

Comment thread R/bm-write-file.R Outdated
Comment thread R/ensure-format.R Outdated
@thisisnic
Copy link
Copy Markdown
Author

Cheers @jonkeane, have now removed a couple of the additions there.

@jonkeane
Copy link
Copy Markdown
Contributor

😢 Apparently I can't approve the CI runs, nor merge to this repo. @assignUser could you assist?

@assignUser
Copy link
Copy Markdown

I don't have write access either, I'll request that asap

@assignUser
Copy link
Copy Markdown

Ugh, looks like there are multiple issues that need to be fixed unrelated to the PR... I assume the env in the actual benchmark runner is pinned better than here?

@thisisnic
Copy link
Copy Markdown
Author

Ugh, looks like there are multiple issues that need to be fixed unrelated to the PR... I assume the env in the actual benchmark runner is pinned better than here?

Unsure sorry. Mind approving the workflows again so I can check my changes worked for at least the tests bit for now?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants