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

[C#] ArrowArrayConcatenator doesn't handle non-zero offsets #41164

Closed
adamreeve opened this issue Apr 12, 2024 · 3 comments
Closed

[C#] ArrowArrayConcatenator doesn't handle non-zero offsets #41164

adamreeve opened this issue Apr 12, 2024 · 3 comments
Assignees
Milestone

Comments

@adamreeve
Copy link
Contributor

Describe the bug, including details regarding any error messages, version, and platform.

Example unit test to reproduce:

[Fact]
public void TestConcatenateSlicedArrays()
{
    Int32Array array0 = new Int32Array.Builder()
        .Append(new []{0, 1, 2})
        .AppendNull()
        .Append(new []{3, 4})
        .Build();
    Int32Array array1 = new Int32Array.Builder()
        .Append(new []{5, 6})
        .AppendNull()
        .Append(new [] {7, 8, 9})
        .Build();

    var concatenated = ArrowArrayConcatenator.Concatenate(new[]
    {
        ArrowArrayFactory.Slice(array0, 2, 3),
        ArrowArrayFactory.Slice(array1, 2, 3),
    });

    var expected = new Int32Array.Builder()
        .Append(2).AppendNull().Append(3)
        .AppendNull().Append(new[] { 7, 8 })
        .Build();

    Assert.Equal(expected, (IReadOnlyList<int?>)concatenated);
}

This fails with:

Assert.Equal() Failure: Collections differ
           ↓ (pos 0)
Expected: [2, null, 3, null, 7, ···]
Actual:   [0, 1, 2, 5, 6, ···]
           ↑ (pos 0)

Component(s)

C#

CurtHagenlocher pushed a commit that referenced this issue 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.
* GitHub Issue: #41164

Authored-by: Adam Reeve <[email protected]>
Signed-off-by: Curt Hagenlocher <[email protected]>
@CurtHagenlocher CurtHagenlocher added this to the 17.0.0 milestone Apr 17, 2024
@CurtHagenlocher
Copy link
Contributor

Issue resolved by pull request 41245
#41245

tolleybot pushed a commit to tmct/arrow that referenced this issue 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 issue 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 issue 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 issue 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 issue 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?

@CurtHagenlocher
Copy link
Contributor

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

The Arrow release process is fairly labor-intensive, so bugfix releases are typically reserved for regressions. This fix is for a longstanding bug and not a regression, so on its own it wouldn't be a candidate for a point release. If it turns out that we need a 16.2 release for some other reason, I'll investigate the possibility of porting this set of fixes alongside it.

The next regular release will be roughly at the beginning of August.

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

No branches or pull requests

3 participants