Skip to content
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

GH-41349: [C#] Optimize DecimalUtility.GetBytes(SqlDecimal) on .NET 7+ #41391

Closed
wants to merge 11 commits into from

Conversation

CurtHagenlocher
Copy link
Contributor

@CurtHagenlocher CurtHagenlocher commented Apr 26, 2024

What changes are included in this PR?

Adds code to avoid an allocation when converting from SqlDecimal to Decimal128.
Adds a .NET 8 target to Apache.Arrow.csproj to enable the optimization.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

Closes #41349

CurtHagenlocher and others added 10 commits May 9, 2024 05:39
…m link libraries (apache#41341)

### Rationale for this change

We should use `.lib` (import library) not `.dll` for linking. So `.dll` is wrong. (But it seems working... Why...?)

### What changes are included in this PR?

Remove `.dll` from link libraries because CMake generates suitable link options from library name automatically:

https://cmake.org/cmake/help/latest/command/target_link_libraries.html#command:target_link_libraries

> A plain library name: The generated link line will ask the linker to search for the library (e.g. `foo` becomes `-lfoo` or `foo.lib`).

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* GitHub Issue: apache#41340

Authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
)

### Rationale for this change

Ruby 2.7 doesn't exist on `macos-latest` (`macos-14`).

### What changes are included in this PR?

Use `ruby` as the Ruby version to use the latest Ruby.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#41371

Authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Raúl Cumplido <[email protected]>
…ing dict (apache#41344)

### Rationale for this change

`dictionary_length = 0` is used when page doesn't have dictionary, however, this should be a nop.

### What changes are included in this PR?

Change `dictionary_length = 0` to `*dictionary_length = 0`.

### Are these changes tested?

No?

### Are there any user-facing changes?

no

Authored-by: mwish <[email protected]>
Signed-off-by: mwish <[email protected]>
…f `macos-latest` change to `macos-14` (apache#41384)

### Rationale for this change

* The MATLAB macOS CI workflow is failing because of the recent change to `macos-latest` from `macos-12` to `macos-14`.
* In https://github.blog/changelog/2024-01-30-github-actions-macos-14-sonoma-is-now-available/ it is mentioned that the transition to `macos-14` was planned for April-June 2024.
* As noted in the [Platform Road Map](https://www.mathworks.com/support/requirements/platform-road-map.html), MATLAB R2023a is not officially supported with `macos-14`.
* Until we can move to building against MATLAB R2024a (latest available version of MATLAB), it makes sense to pin back the macOS version to version 14.

### What changes are included in this PR?

1. Pin back MATLAB CI workflow to use `macos-12`.
2. Pin back MATLAB crossbow packaging workflow to use `macos-12`.

### Are these changes tested?

I will run a crossbow job and ensure that the MATLAB CI workflow passes successfully before merging this PR.

### Are there any user-facing changes?

No.

### Future Directions

1.  apache#41385

### Notes

1. Thanks @ sgilmore10 for your help with this pull request!
* GitHub Issue: apache#41370

Authored-by: Kevin Gurney <[email protected]>
Signed-off-by: Kevin Gurney <[email protected]>
### Rationale for this change

We don't have write permission for `/usr/local` on macos-14.

### What changes are included in this PR?

Use `/tmp/local` instead.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#41369

Authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
)

### Rationale for this change

It's useful to detect type difference.

### What changes are included in this PR?

Add `:show_column_type` option to `Arrow::Table#to_s` and enables it by default.

This is a backward incompatible change but this'll help users.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.

**This PR includes breaking changes to public APIs.**

* GitHub Issue: apache#41327

Authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
…obuf.cmake (apache#41360)

### Rationale for this change

`protobuf::libprotobuf` provided by `FindProtobuf.cmake` (provided by CMake) may not provide needed dependencies such as Abseil.

### What changes are included in this PR?

Try `protobuf-config.cmake` provided by Protobuf before `FindProtobuf.cmake`.
`protobuf::libprotobuf` provided by `protobuf-config.cmake` must have needed dependencies.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* GitHub Issue: apache#41333

Authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
…t if it exists (apache#41305)

### Rationale for this change

When we created the `16.1.0` milestone the merge script started prompting it instead of `17.0.0` we want to default to the next major release.

### What changes are included in this PR?

Update archery logic to default to major versions online.

### Are these changes tested?
I've tested locally and now it defaults to `17.0.0`:

```
Enter fix version [17.0.0]: 
```

### Are there any user-facing changes?
No
* GitHub Issue: apache#41282

Authored-by: Raúl Cumplido <[email protected]>
Signed-off-by: Raúl Cumplido <[email protected]>
### What changes are included in this PR?

Changes to workflow infrastructure and projects to install and target net8.0 instead of net7.0.

### Are these changes tested?

Yes

### Are there any user-facing changes?

Users will need to install .NET 8 to run tests and examples. No impact on product code.

Closes apache#41375
* GitHub Issue: apache#41375

Lead-authored-by: Curt Hagenlocher <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Curt Hagenlocher <[email protected]>
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.

[C#] use SqlDecimal.WriteTdsValue for .NET 7+ to avoid allocating a byte[] array
5 participants