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-41164: [C#] Fix concatenation of sliced arrays #41245

Merged
merged 11 commits into from
Apr 17, 2024

Conversation

adamreeve
Copy link
Contributor

@adamreeve adamreeve commented Apr 17, 2024

Rationale for this change

Makes array concatenation work correctly when the input arrays have been sliced.

What changes are included in this PR?

  • Updates the array concatenation tests so that the TestDataGenerator can generate test cases with sliced input arrays. To avoid too much duplicated logic, I've added a new GenerateTestData<TArray, TArrayBuilder> method that works with builders that are not IArrowArrayBuilder<T, TArray, TArrayBuilder>, and simplified a lot of the data generation by using this new method. Only struct and union array test data generation still needs to duplicate the logic in GenerateTestData.
  • Fixes ArrayDataConcatenator logic to handle sliced input arrays

Are these changes tested?

Yes, I've added a new test for this.

Are there any user-facing changes?

Yes, this is a user-facing bug fix.

Copy link

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


namespace Apache.Arrow.Tests
{
static class ArrayArrayBuilderFactoryReflector
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like this was originally added as a way to access the internal ArrowArrayBuilderFactory class, but since it was added, an InternalsVisibleTo("Apache.Arrow.Tests") assembly attribute has been added to Apache.Arrow, so this is no longer needed.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 17, 2024
@CurtHagenlocher CurtHagenlocher merged commit 63c91ff into apache:main Apr 17, 2024
9 checks passed
@CurtHagenlocher CurtHagenlocher removed the awaiting committer review Awaiting committer review label Apr 17, 2024
@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Apr 17, 2024
@adamreeve adamreeve deleted the slice_concatenation branch April 17, 2024 04:04
Copy link

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 63c91ff.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

tolleybot pushed a commit to tmct/arrow that referenced this pull request May 2, 2024
### Rationale for this change

Makes array concatenation work correctly when the input arrays have been sliced.

### What changes are included in this PR?

* Updates the array concatenation tests so that the `TestDataGenerator` can generate test cases with sliced input arrays. To avoid too much duplicated logic, I've added a new `GenerateTestData<TArray, TArrayBuilder>` method that works with builders that are not `IArrowArrayBuilder<T, TArray, TArrayBuilder>`, and simplified a lot of the data generation by using this new method. Only struct and union array test data generation still needs to duplicate the logic in `GenerateTestData`.
* Fixes `ArrayDataConcatenator` logic to handle sliced input arrays

### Are these changes tested?

Yes, I've added a new test for this.

### Are there any user-facing changes?

Yes, this is a user-facing bug fix.
* GitHub Issue: apache#41164

Authored-by: Adam Reeve <[email protected]>
Signed-off-by: Curt Hagenlocher <[email protected]>
tolleybot pushed a commit to tmct/arrow that referenced this pull request May 4, 2024
### Rationale for this change

Makes array concatenation work correctly when the input arrays have been sliced.

### What changes are included in this PR?

* Updates the array concatenation tests so that the `TestDataGenerator` can generate test cases with sliced input arrays. To avoid too much duplicated logic, I've added a new `GenerateTestData<TArray, TArrayBuilder>` method that works with builders that are not `IArrowArrayBuilder<T, TArray, TArrayBuilder>`, and simplified a lot of the data generation by using this new method. Only struct and union array test data generation still needs to duplicate the logic in `GenerateTestData`.
* Fixes `ArrayDataConcatenator` logic to handle sliced input arrays

### Are these changes tested?

Yes, I've added a new test for this.

### Are there any user-facing changes?

Yes, this is a user-facing bug fix.
* GitHub Issue: apache#41164

Authored-by: Adam Reeve <[email protected]>
Signed-off-by: Curt Hagenlocher <[email protected]>
rok pushed a commit to tmct/arrow that referenced this pull request May 8, 2024
### Rationale for this change

Makes array concatenation work correctly when the input arrays have been sliced.

### What changes are included in this PR?

* Updates the array concatenation tests so that the `TestDataGenerator` can generate test cases with sliced input arrays. To avoid too much duplicated logic, I've added a new `GenerateTestData<TArray, TArrayBuilder>` method that works with builders that are not `IArrowArrayBuilder<T, TArray, TArrayBuilder>`, and simplified a lot of the data generation by using this new method. Only struct and union array test data generation still needs to duplicate the logic in `GenerateTestData`.
* Fixes `ArrayDataConcatenator` logic to handle sliced input arrays

### Are these changes tested?

Yes, I've added a new test for this.

### Are there any user-facing changes?

Yes, this is a user-facing bug fix.
* GitHub Issue: apache#41164

Authored-by: Adam Reeve <[email protected]>
Signed-off-by: Curt Hagenlocher <[email protected]>
rok pushed a commit to tmct/arrow that referenced this pull request May 8, 2024
### Rationale for this change

Makes array concatenation work correctly when the input arrays have been sliced.

### What changes are included in this PR?

* Updates the array concatenation tests so that the `TestDataGenerator` can generate test cases with sliced input arrays. To avoid too much duplicated logic, I've added a new `GenerateTestData<TArray, TArrayBuilder>` method that works with builders that are not `IArrowArrayBuilder<T, TArray, TArrayBuilder>`, and simplified a lot of the data generation by using this new method. Only struct and union array test data generation still needs to duplicate the logic in `GenerateTestData`.
* Fixes `ArrayDataConcatenator` logic to handle sliced input arrays

### Are these changes tested?

Yes, I've added a new test for this.

### Are there any user-facing changes?

Yes, this is a user-facing bug fix.
* GitHub Issue: apache#41164

Authored-by: Adam Reeve <[email protected]>
Signed-off-by: Curt Hagenlocher <[email protected]>
vibhatha pushed a commit to vibhatha/arrow that referenced this pull request May 25, 2024
### Rationale for this change

Makes array concatenation work correctly when the input arrays have been sliced.

### What changes are included in this PR?

* Updates the array concatenation tests so that the `TestDataGenerator` can generate test cases with sliced input arrays. To avoid too much duplicated logic, I've added a new `GenerateTestData<TArray, TArrayBuilder>` method that works with builders that are not `IArrowArrayBuilder<T, TArray, TArrayBuilder>`, and simplified a lot of the data generation by using this new method. Only struct and union array test data generation still needs to duplicate the logic in `GenerateTestData`.
* Fixes `ArrayDataConcatenator` logic to handle sliced input arrays

### Are these changes tested?

Yes, I've added a new test for this.

### Are there any user-facing changes?

Yes, this is a user-facing bug fix.
* GitHub Issue: apache#41164

Authored-by: Adam Reeve <[email protected]>
Signed-off-by: Curt Hagenlocher <[email protected]>
@nmishturak-gl
Copy link

Can this be backported to a bugfix 16.x release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants