[GitHub] [arrow] lidavidm commented on a diff in pull request #14126: GH-32947: [Go] Implement Concatenate for RLE Array

["lidavidm (via GitHub)" <gi...@apache.org>]

lidavidm commented on code in PR #14126:
URL: https://github.com/apache/arrow/pull/14126#discussion_r1086698410


##########
go/arrow/array/concat_test.go:
##########
@@ -568,3 +569,59 @@ func TestConcatDictionaryNullSlots(t *testing.T) {
 
 	assert.Truef(t, array.Equal(actual, expected), "got: %s, expected: %s", actual, expected)
 }
+
+func TestConcatRunLengthEncoded(t *testing.T) {

Review Comment:
   I suppose:
   
   - Can we test all three index types?
   - Can we test that we error properly on overflow?
   - Can we test that we can concatenate right up to the edge of overflow?



##########
go/arrow/array/concat.go:
##########
@@ -517,6 +523,50 @@ func concat(data []arrow.ArrayData, mem memory.Allocator) (arrow.ArrayData, erro
 		if err != nil {
 			return nil, err
 		}
+	case *arrow.RunEndEncodedType:
+		physicalLength, overflow := int(0), false
+		// we can't use gatherChildren because the Offset and Len of
+		// data doesn't correspond to the physical length or offset
+		runs := make([]arrow.ArrayData, len(data))
+		values := make([]arrow.ArrayData, len(data))
+		for i, d := range data {
+			plen := encoded.GetPhysicalLength(d)
+			off := encoded.FindPhysicalOffset(d)
+
+			runs[i] = NewSliceData(d.Children()[0], int64(off), int64(off+plen))
+			defer runs[i].Release()
+			values[i] = NewSliceData(d.Children()[1], int64(off), int64(off+plen))
+			defer values[i].Release()
+
+			physicalLength, overflow = addOvf(physicalLength, int(plen))
+			if overflow {
+				return nil, fmt.Errorf("%w: run length encoded array length must fit into a 32-bit signed integer",

Review Comment:
   I suppose there are separate checks below, and since lengths are limited to 32-bit...



##########
go/arrow/array/concat.go:
##########
@@ -517,6 +523,50 @@ func concat(data []arrow.ArrayData, mem memory.Allocator) (arrow.ArrayData, erro
 		if err != nil {
 			return nil, err
 		}
+	case *arrow.RunEndEncodedType:
+		physicalLength, overflow := int(0), false
+		// we can't use gatherChildren because the Offset and Len of
+		// data doesn't correspond to the physical length or offset
+		runs := make([]arrow.ArrayData, len(data))
+		values := make([]arrow.ArrayData, len(data))
+		for i, d := range data {
+			plen := encoded.GetPhysicalLength(d)
+			off := encoded.FindPhysicalOffset(d)
+
+			runs[i] = NewSliceData(d.Children()[0], int64(off), int64(off+plen))
+			defer runs[i].Release()
+			values[i] = NewSliceData(d.Children()[1], int64(off), int64(off+plen))
+			defer values[i].Release()
+
+			physicalLength, overflow = addOvf(physicalLength, int(plen))
+			if overflow {
+				return nil, fmt.Errorf("%w: run length encoded array length must fit into a 32-bit signed integer",

Review Comment:
   Hmm, shouldn't this depend on the offset type?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org