You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "bkietz (via GitHub)" <gi...@apache.org> on 2023/12/01 01:40:02 UTC

[PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

bkietz opened a new pull request, #39019:
URL: https://github.com/apache/arrow/pull/39019

   
   
   <!--
   Thanks for opening a pull request!
   If this is your first pull request you can find detailed information on how 
   to contribute here:
     * [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
     * [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
   
   
   If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
   
   Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
   
   Then could you also rename the pull request title in the following format?
   
       GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
   
   or
   
       MINOR: [${COMPONENT}] ${SUMMARY}
   
   In the case of PARQUET issues on JIRA the title also supports:
   
       PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
   
   -->
   
   ### Rationale for this change
   
   The Go implementation should support import/export of the new data types. This will enable integration testing between the C++ and Go implementations.
   
   <!--
    Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
    Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.  
   -->
   
   ### What changes are included in this PR?
   
   Added import/export for the new data types and arrays of data of those types.
   
   <!--
   There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
   -->
   
   ### Are these changes tested?
   
   Yes, they will be covered by the integration tests and existing Go unit tests.
   
   <!--
   We typically require tests for all PRs in order to:
   1. Prevent the code from being accidentally broken by subsequent changes
   2. Serve as another way to document the expected behavior of the code
   
   If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
   -->
   
   ### Are there any user-facing changes?
   
   This is a user facing change
   
   <!--
   If there are user-facing changes then we may require documentation to be updated before approving the PR.
   -->
   
   <!--
   If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
   -->
   <!-- **This PR includes breaking changes to public APIs.** -->
   
   <!--
   Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
   -->
   <!-- **This PR contains a "Critical Fix".** -->


-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1427990144


##########
go/arrow/internal/arrjson/arrjson.go:
##########
@@ -1090,7 +1091,7 @@ func arrayFromJSON(mem memory.Allocator, dt arrow.DataType, arr Array) arrow.Arr
 	case arrow.BinaryViewDataType:
 		valids := validsToBitmap(validsFromJSON(arr.Valids), mem)
 		nulls := arr.Count - bitutil.CountSetBits(valids.Bytes(), 0, arr.Count)
-		headers := stringHeadersFromJSON(mem, !dt.IsUtf8(), arr.Data)
+		headers := stringHeadersFromJSON(mem, !dt.IsUtf8(), arr.Views)

Review Comment:
   Nope, will add
   



##########
go/arrow/cdata/cdata.go:
##########
@@ -654,6 +680,40 @@ func (imp *cimporter) importStringLike(offsetByteWidth int64) (err error) {
 	return
 }
 
+func (imp *cimporter) importBinaryViewLike() (err error) {
+	if err = imp.checkNoChildren(); err != nil {
+		return
+	}
+
+	buffers := make([]*memory.Buffer, len(imp.cbuffers)-1)
+	// XXX couldn't figure out how to extract file_reader.go::releaseBuffers as a utility
+	defer func() {
+		for _, buf := range buffers {
+			if buf != nil {
+				buf.Release()
+			}
+		}
+	}()

Review Comment:
   I'll try that
   



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1425719213


##########
go/arrow/array/list.go:
##########
@@ -1411,217 +1410,56 @@ func (b *baseListViewBuilder) UnmarshalJSON(data []byte) error {
 	return b.Unmarshal(dec)
 }
 
-// Pre-conditions:
-//
-//	input.DataType() is ListViewType
-//	input.Len() > 0 && input.NullN() != input.Len()
-func minListViewOffset32(input arrow.ArrayData) int32 {
-	var bitmap []byte
-	if input.Buffers()[0] != nil {
-		bitmap = input.Buffers()[0].Bytes()
-	}
-	offsets := arrow.Int32Traits.CastFromBytes(input.Buffers()[1].Bytes())[input.Offset():]
-	sizes := arrow.Int32Traits.CastFromBytes(input.Buffers()[2].Bytes())[input.Offset():]
-
-	isNull := func(i int) bool {
-		return bitmap != nil && bitutil.BitIsNotSet(bitmap, input.Offset()+i)
-	}
-
-	// It's very likely that the first non-null non-empty list-view starts at
-	// offset 0 of the child array.
-	i := 0
-	for i < input.Len() && (isNull(i) || sizes[i] == 0) {
-		i += 1
-	}
-	if i >= input.Len() {
-		return 0
-	}
-	minOffset := offsets[i]
-	if minOffset == 0 {
-		// early exit: offset 0 found already
-		return 0
-	}
-
-	// Slow path: scan the buffers entirely.
-	i += 1
-	for ; i < input.Len(); i += 1 {
-		if isNull(i) {
-			continue
-		}
-		offset := offsets[i]
-		if offset < minOffset && sizes[i] > 0 {
-			minOffset = offset
-		}
-	}
-	return minOffset
-}
-
-// Find the maximum offset+size in a LIST_VIEW array.
+// Find the minimum offset+size in a LIST_VIEW/LARGE_LIST_VIEW array.
 //
 // Pre-conditions:
 //
-//	input.DataType() is ListViewType
-//	input.Len() > 0 && input.NullN() != input.Len()
-func maxListViewOffset32(input arrow.ArrayData) int {
+//	input.DataType() is ListViewType if Offset=int32 or LargeListViewType if Offset=int64
+//	input.Len() > 0

Review Comment:
   Why remove the `NullN() != Len()` pre-condition?



##########
go/arrow/array/list.go:
##########
@@ -1411,217 +1410,56 @@ func (b *baseListViewBuilder) UnmarshalJSON(data []byte) error {
 	return b.Unmarshal(dec)
 }
 
-// Pre-conditions:
-//
-//	input.DataType() is ListViewType
-//	input.Len() > 0 && input.NullN() != input.Len()
-func minListViewOffset32(input arrow.ArrayData) int32 {
-	var bitmap []byte
-	if input.Buffers()[0] != nil {
-		bitmap = input.Buffers()[0].Bytes()
-	}
-	offsets := arrow.Int32Traits.CastFromBytes(input.Buffers()[1].Bytes())[input.Offset():]
-	sizes := arrow.Int32Traits.CastFromBytes(input.Buffers()[2].Bytes())[input.Offset():]
-
-	isNull := func(i int) bool {
-		return bitmap != nil && bitutil.BitIsNotSet(bitmap, input.Offset()+i)
-	}
-
-	// It's very likely that the first non-null non-empty list-view starts at
-	// offset 0 of the child array.
-	i := 0
-	for i < input.Len() && (isNull(i) || sizes[i] == 0) {
-		i += 1
-	}
-	if i >= input.Len() {
-		return 0
-	}
-	minOffset := offsets[i]
-	if minOffset == 0 {
-		// early exit: offset 0 found already
-		return 0
-	}
-
-	// Slow path: scan the buffers entirely.
-	i += 1
-	for ; i < input.Len(); i += 1 {
-		if isNull(i) {
-			continue
-		}
-		offset := offsets[i]
-		if offset < minOffset && sizes[i] > 0 {
-			minOffset = offset
-		}
-	}
-	return minOffset
-}
-
-// Find the maximum offset+size in a LIST_VIEW array.
+// Find the minimum offset+size in a LIST_VIEW/LARGE_LIST_VIEW array.
 //
 // Pre-conditions:
 //
-//	input.DataType() is ListViewType
-//	input.Len() > 0 && input.NullN() != input.Len()
-func maxListViewOffset32(input arrow.ArrayData) int {
+//	input.DataType() is ListViewType if Offset=int32 or LargeListViewType if Offset=int64
+//	input.Len() > 0
+func minListViewOffset[Offset int32 | int64](input arrow.ArrayData) Offset {
 	inputOffset := input.Offset()
-	var bitmap []byte
-	if input.Buffers()[0] != nil {
-		bitmap = input.Buffers()[0].Bytes()
-	}
-	offsets := arrow.Int32Traits.CastFromBytes(input.Buffers()[1].Bytes())[inputOffset:]
-	sizes := arrow.Int32Traits.CastFromBytes(input.Buffers()[2].Bytes())[inputOffset:]
-
-	isNull := func(i int) bool {
-		return bitmap != nil && bitutil.BitIsNotSet(bitmap, inputOffset+i)
-	}
-
-	i := input.Len() - 1 // safe because input.Len() > 0
-	for i != 0 && (isNull(i) || sizes[i] == 0) {
-		i -= 1
-	}
-	offset := offsets[i]
-	size := sizes[i]
-	if i == 0 {
-		if isNull(i) || sizes[i] == 0 {
-			return 0
-		} else {
-			return int(offset + size)
-		}
-	}
+	offsets := arrow.GetData[Offset](input.Buffers()[1].Bytes())[inputOffset:]
 
-	values := input.Children()[0]
-	maxEnd := int(offsets[i] + sizes[i])
-	if maxEnd == values.Len() {
-		// Early-exit: maximum possible view-end found already.
-		return maxEnd
-	}
-
-	// Slow path: scan the buffers entirely.
-	for ; i >= 0; i -= 1 {
-		offset := offsets[i]
-		size := sizes[i]
-		if size > 0 && !isNull(i) {
-			if int(offset+size) > maxEnd {
-				maxEnd = int(offset + size)
-				if maxEnd == values.Len() {
-					return maxEnd
-				}
-			}
-		}
-	}
-	return maxEnd
-}
-
-// Pre-conditions:
-//
-//	input.DataType() is LargeListViewType
-//	input.Len() > 0 && input.NullN() != input.Len()
-func minLargeListViewOffset64(input arrow.ArrayData) int64 {
-	var bitmap []byte
-	if input.Buffers()[0] != nil {
-		bitmap = input.Buffers()[0].Bytes()
-	}
-	offsets := arrow.Int64Traits.CastFromBytes(input.Buffers()[1].Bytes())[input.Offset():]
-	sizes := arrow.Int64Traits.CastFromBytes(input.Buffers()[2].Bytes())[input.Offset():]
-
-	isNull := func(i int) bool {
-		return bitmap != nil && bitutil.BitIsNotSet(bitmap, input.Offset()+i)
-	}
-
-	// It's very likely that the first non-null non-empty list-view starts at
-	// offset 0 of the child array.
 	i := 0
-	for i < input.Len() && (isNull(i) || sizes[i] == 0) {
-		i += 1
-	}
-	if i >= input.Len() {
-		return 0
-	}
-	minOffset := offsets[i]
-	if minOffset == 0 {
-		// early exit: offset 0 found already
-		return 0
-	}
+	minOffset := offsets[i] // safe because input.Len() > 0
 
-	// Slow path: scan the buffers entirely.
-	i += 1
-	for ; i < input.Len(); i += 1 {
-		if isNull(i) {
-			continue
+	for i += 1; i < input.Len(); i += 1 {
+		if minOffset == 0 {
+			// Fast path: the minimum offset is frequently 0 (the start of the child array),
+			// and frequently a view which has this offset will be near the start of the array.
+			return 0
 		}
-		offset := offsets[i]
-		if offset < minOffset && sizes[i] > 0 {
+		if offset := offsets[i]; offset < minOffset {
 			minOffset = offset
 		}
 	}
 	return minOffset
 }
 
-// Find the maximum offset+size in a LARGE_LIST_VIEW array.
+// Find the maximum offset+size in a LIST_VIEW/LARGE_LIST_VIEW array.
 //
 // Pre-conditions:
 //
-//	input.DataType() is LargeListViewType
-//	input.Len() > 0 && input.NullN() != input.Len()
-func maxLargeListViewOffset64(input arrow.ArrayData) int64 {
+//	input.DataType() is ListViewType if Offset=int32 or LargeListViewType if Offset=int64
+//	input.Len() > 0
+func maxListViewEnd[Offset int32 | int64](input arrow.ArrayData) Offset {
 	inputOffset := input.Offset()
-	var bitmap []byte
-	if input.Buffers()[0] != nil {
-		bitmap = input.Buffers()[0].Bytes()
-	}
-	offsets := arrow.Int64Traits.CastFromBytes(input.Buffers()[1].Bytes())[inputOffset:]
-	sizes := arrow.Int64Traits.CastFromBytes(input.Buffers()[2].Bytes())[inputOffset:]
+	offsets := arrow.GetData[Offset](input.Buffers()[1].Bytes())[inputOffset:]
+	sizes := arrow.GetData[Offset](input.Buffers()[2].Bytes())[inputOffset:]
 
-	isNull := func(i int) bool {
-		return bitmap != nil && bitutil.BitIsNotSet(bitmap, inputOffset+i)
-	}
+	maxLegalOffset := Offset(input.Children()[0].Len())
 
-	// It's very likely that the first non-null non-empty list-view starts at
-	// offset zero, so we check that first and potentially early-return a 0.
-	i := input.Len() - 1 // safe because input.Len() > 0
-	for i != 0 && (isNull(i) || sizes[i] == 0) {
-		i -= 1
-	}
-	offset := offsets[i]
-	size := sizes[i]
-	if i == 0 {
-		if isNull(i) || sizes[i] == 0 {
-			return 0
-		} else {
-			return offset + size
-		}
-	}
+	i := input.Len() - 1
+	maxEnd := offsets[i] + sizes[i] // safe because input.Len() > 0
 
-	if offset > math.MaxInt64-size {
-		// Early-exit: 64-bit overflow detected. This is not possible on a
-		// valid list-view, but we return the maximum possible value to
-		// avoid undefined behavior.
-		return math.MaxInt64
-	}
-	values := input.Children()[0]
-	maxEnd := offsets[i] + sizes[i]
-	if maxEnd == int64(values.Len()) {
-		// Early-exit: maximum possible view-end found already.
-		return maxEnd
-	}
-
-	// Slow path: scan the buffers entirely.
-	for ; i >= 0; i -= 1 {
-		offset := offsets[i]
-		size := sizes[i]
-		if size > 0 && !isNull(i) {
-			if offset+size > maxEnd {
-				if offset > math.MaxInt64-size {
-					// 64-bit overflow detected. This is not possible on a valid list-view,
-					// but we saturate maxEnd to the maximum possible value to avoid
-					// undefined behavior.
-					return math.MaxInt64
-				}
-				maxEnd = offset + size
-				if maxEnd == int64(values.Len()) {
-					return maxEnd
-				}
-			}
+	for i -= 1; i >= 0; i -= 1 {
+		if maxEnd == maxLegalOffset {
+			// Fast path: the maximum offset+size is frequently exactly the end of the child array,
+			// and frequently a view which has this offset+size will be near the end of the array.
+			return maxEnd
+		}
+		if end := offsets[i] + sizes[i]; end > maxEnd {
+			maxEnd = end

Review Comment:
   You can take a look at my final version of this function in the C++ implementation: 
   
   https://github.com/apache/arrow/blob/main/cpp/src/arrow/util/list_util.cc#L91



##########
dev/archery/archery/integration/datagen.py:
##########
@@ -927,6 +927,85 @@ class LargeListColumn(_BaseListColumn, _LargeOffsetsMixin):
     pass
 
 
+class ListViewField(Field):
+
+    def __init__(self, name, value_field, *, nullable=True,
+                 metadata=None):
+        super().__init__(name, nullable=nullable,
+                         metadata=metadata)
+        self.value_field = value_field
+
+    @property
+    def column_class(self):
+        return ListViewColumn
+
+    def _get_type(self):
+        return OrderedDict([
+            ('name', 'listview')
+        ])
+
+    def _get_children(self):
+        return [self.value_field.get_json()]
+
+    def generate_column(self, size, name=None):
+        MAX_LIST_SIZE = 4
+
+        is_valid = self._make_is_valid(size)
+        offsets = []
+        sizes = np.random.randint(0, MAX_LIST_SIZE + 1, size=size)
+        offset = 0
+        for s in sizes:
+            offsets.append(offset)
+            offset += int(s)

Review Comment:
   Offsets should probably be out of order to break code that assumes list-view offsets are sorted like in lists.



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1425694841


##########
dev/archery/archery/integration/runner.py:
##########
@@ -193,6 +193,8 @@ def _run_test_cases(self,
         ``case_runner`` ran against ``test_cases``
         """
         def case_wrapper(test_case):
+            if serial:
+                return case_runner(test_case)

Review Comment:
   Yes: printer.cork() was failing to dump output when the test segfaulted. In any case, corking the printer adds no value when we're running in serial.



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39019:
URL: https://github.com/apache/arrow/pull/39019#issuecomment-1864226532

   After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 3e182f2c9d5c710ce809e342f5c8cee547f979d2.
   
   There were no benchmark performance regressions. 🎉
   
   The [full Conbench report](https://github.com/apache/arrow/runs/19819326798) has more details.


-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1428176260


##########
go/arrow/array/list.go:
##########
@@ -1411,217 +1410,56 @@ func (b *baseListViewBuilder) UnmarshalJSON(data []byte) error {
 	return b.Unmarshal(dec)
 }
 
-// Pre-conditions:
-//
-//	input.DataType() is ListViewType
-//	input.Len() > 0 && input.NullN() != input.Len()
-func minListViewOffset32(input arrow.ArrayData) int32 {
-	var bitmap []byte
-	if input.Buffers()[0] != nil {
-		bitmap = input.Buffers()[0].Bytes()
-	}
-	offsets := arrow.Int32Traits.CastFromBytes(input.Buffers()[1].Bytes())[input.Offset():]
-	sizes := arrow.Int32Traits.CastFromBytes(input.Buffers()[2].Bytes())[input.Offset():]
-
-	isNull := func(i int) bool {
-		return bitmap != nil && bitutil.BitIsNotSet(bitmap, input.Offset()+i)
-	}
-
-	// It's very likely that the first non-null non-empty list-view starts at
-	// offset 0 of the child array.
-	i := 0
-	for i < input.Len() && (isNull(i) || sizes[i] == 0) {
-		i += 1
-	}
-	if i >= input.Len() {
-		return 0
-	}
-	minOffset := offsets[i]
-	if minOffset == 0 {
-		// early exit: offset 0 found already
-		return 0
-	}
-
-	// Slow path: scan the buffers entirely.
-	i += 1
-	for ; i < input.Len(); i += 1 {
-		if isNull(i) {
-			continue
-		}
-		offset := offsets[i]
-		if offset < minOffset && sizes[i] > 0 {
-			minOffset = offset
-		}
-	}
-	return minOffset
-}
-
-// Find the maximum offset+size in a LIST_VIEW array.
+// Find the minimum offset+size in a LIST_VIEW/LARGE_LIST_VIEW array.
 //
 // Pre-conditions:
 //
-//	input.DataType() is ListViewType
-//	input.Len() > 0 && input.NullN() != input.Len()
-func maxListViewOffset32(input arrow.ArrayData) int {
+//	input.DataType() is ListViewType if Offset=int32 or LargeListViewType if Offset=int64
+//	input.Len() > 0

Review Comment:
   This is why it's important to keep the documentation about the pre-condition `... && input.NullN() != input.Len()`. The concept of "minumum offset" is undefined when there are 0 offsets to look at (an empty set can't have a minimum) -- the caller can betetr decide what's appropriate to do when all values are null.



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1427335176


##########
go/arrow/internal/flatbuf/Binary.go:
##########
@@ -22,7 +22,7 @@ import (
 	flatbuffers "github.com/google/flatbuffers/go"
 )
 
-/// Opaque binary data
+// / Opaque binary data

Review Comment:
   I'll revert most of these



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #39019:
URL: https://github.com/apache/arrow/pull/39019#issuecomment-1858442967

   @pitrou any further issues here / remaining things to be addressed?


-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1427382070


##########
go/arrow/cdata/cdata.go:
##########
@@ -654,6 +680,40 @@ func (imp *cimporter) importStringLike(offsetByteWidth int64) (err error) {
 	return
 }
 
+func (imp *cimporter) importBinaryViewLike() (err error) {
+	if err = imp.checkNoChildren(); err != nil {
+		return
+	}
+
+	buffers := make([]*memory.Buffer, len(imp.cbuffers)-1)
+	// XXX couldn't figure out how to extract file_reader.go::releaseBuffers as a utility
+	defer func() {
+		for _, buf := range buffers {
+			if buf != nil {
+				buf.Release()
+			}
+		}
+	}()

Review Comment:
   we could export it (just make the first letter capitalized) but if we do that, it might make more sense for it to exist inside of `memory` as a utility rather than from the `ipc` package



##########
go/arrow/type_traits.go:
##########
@@ -0,0 +1,147 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package arrow
+
+import (
+	"reflect"
+	"unsafe"
+
+	"github.com/apache/arrow/go/v15/arrow/decimal128"
+	"github.com/apache/arrow/go/v15/arrow/decimal256"
+	"github.com/apache/arrow/go/v15/arrow/float16"
+	"golang.org/x/exp/constraints"
+)
+
+// IntType is a type constraint for raw values represented as signed
+// integer types by  We aren't just using constraints.Signed
+// because we don't want to include the raw `int` type here whose size
+// changes based on the architecture (int32 on 32-bit architectures and
+// int64 on 64-bit architectures).
+//
+// This will also cover types like MonthInterval or the time types
+// as their underlying types are int32 and int64 which will get covered
+// by using the ~
+type IntType interface {
+	~int8 | ~int16 | ~int32 | ~int64
+}
+
+// UintType is a type constraint for raw values represented as unsigned
+// integer types by  We aren't just using constraints.Unsigned
+// because we don't want to include the raw `uint` type here whose size
+// changes based on the architecture (uint32 on 32-bit architectures and
+// uint64 on 64-bit architectures). We also don't want to include uintptr
+type UintType interface {
+	~uint8 | ~uint16 | ~uint32 | ~uint64
+}
+
+// FloatType is a type constraint for raw values for representing
+// floating point values in  This consists of constraints.Float and
+// float16.Num
+type FloatType interface {
+	float16.Num | constraints.Float
+}
+
+// NumericType is a type constraint for just signed/unsigned integers
+// and float32/float64.
+type NumericType interface {
+	IntType | UintType | constraints.Float
+}
+
+// FixedWidthType is a type constraint for raw values in Arrow that
+// can be represented as FixedWidth byte slices. Specifically this is for
+// using Go generics to easily re-type a byte slice to a properly-typed
+// slice. Booleans are excluded here since they are represented by Arrow
+// as a bitmap and thus the buffer can't be just reinterpreted as a []bool
+type FixedWidthType interface {
+	IntType | UintType |
+		FloatType | decimal128.Num | decimal256.Num |
+		DayTimeInterval | MonthDayNanoInterval
+}
+
+type TemporalType interface {
+	Date32 | Date64 | Time32 | Time64 |
+		Timestamp | Duration | DayTimeInterval |
+		MonthInterval | MonthDayNanoInterval
+}
+
+func sliceAs[Out, T interface{}](b []T) []Out {

Review Comment:
   just to be cleaner, use `any` instead of `interface{}`. I prefer it aesthetically



##########
go/arrow/internal/arrjson/arrjson.go:
##########
@@ -1090,7 +1091,7 @@ func arrayFromJSON(mem memory.Allocator, dt arrow.DataType, arr Array) arrow.Arr
 	case arrow.BinaryViewDataType:
 		valids := validsToBitmap(validsFromJSON(arr.Valids), mem)
 		nulls := arr.Count - bitutil.CountSetBits(valids.Bytes(), 0, arr.Count)
-		headers := stringHeadersFromJSON(mem, !dt.IsUtf8(), arr.Data)
+		headers := stringHeadersFromJSON(mem, !dt.IsUtf8(), arr.Views)

Review Comment:
   was this a change in the integration testing spec?



##########
go/arrow/type_traits.go:
##########
@@ -0,0 +1,147 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package arrow
+
+import (
+	"reflect"
+	"unsafe"
+
+	"github.com/apache/arrow/go/v15/arrow/decimal128"
+	"github.com/apache/arrow/go/v15/arrow/decimal256"
+	"github.com/apache/arrow/go/v15/arrow/float16"
+	"golang.org/x/exp/constraints"
+)
+
+// IntType is a type constraint for raw values represented as signed
+// integer types by  We aren't just using constraints.Signed
+// because we don't want to include the raw `int` type here whose size
+// changes based on the architecture (int32 on 32-bit architectures and
+// int64 on 64-bit architectures).
+//
+// This will also cover types like MonthInterval or the time types
+// as their underlying types are int32 and int64 which will get covered
+// by using the ~
+type IntType interface {
+	~int8 | ~int16 | ~int32 | ~int64
+}
+
+// UintType is a type constraint for raw values represented as unsigned
+// integer types by  We aren't just using constraints.Unsigned
+// because we don't want to include the raw `uint` type here whose size
+// changes based on the architecture (uint32 on 32-bit architectures and
+// uint64 on 64-bit architectures). We also don't want to include uintptr
+type UintType interface {
+	~uint8 | ~uint16 | ~uint32 | ~uint64
+}
+
+// FloatType is a type constraint for raw values for representing
+// floating point values in  This consists of constraints.Float and
+// float16.Num
+type FloatType interface {
+	float16.Num | constraints.Float
+}
+
+// NumericType is a type constraint for just signed/unsigned integers
+// and float32/float64.
+type NumericType interface {
+	IntType | UintType | constraints.Float
+}
+
+// FixedWidthType is a type constraint for raw values in Arrow that
+// can be represented as FixedWidth byte slices. Specifically this is for
+// using Go generics to easily re-type a byte slice to a properly-typed
+// slice. Booleans are excluded here since they are represented by Arrow
+// as a bitmap and thus the buffer can't be just reinterpreted as a []bool
+type FixedWidthType interface {
+	IntType | UintType |
+		FloatType | decimal128.Num | decimal256.Num |
+		DayTimeInterval | MonthDayNanoInterval
+}
+
+type TemporalType interface {
+	Date32 | Date64 | Time32 | Time64 |
+		Timestamp | Duration | DayTimeInterval |
+		MonthInterval | MonthDayNanoInterval
+}
+
+func sliceAs[Out, T interface{}](b []T) []Out {
+  len_bytes := len(b) * int(unsafe.Sizeof(b[0]))
+  cap_bytes := cap(b) * int(unsafe.Sizeof(b[0]))
+
+  var z Out
+  len_out := len_bytes / int(unsafe.Sizeof(z))
+  cap_out := cap_bytes / int(unsafe.Sizeof(z))

Review Comment:
   go typically prefers pascalCase rather than snake_case



##########
go/arrow/internal/utils.go:
##########
@@ -45,3 +45,17 @@ func HasValidityBitmap(id arrow.Type, version flatbuf.MetadataVersion) bool {
 	}
 	return true
 }
+
+// HasBufferSizesBuffer returns whether a given type has an extra buffer
+// in the C ABI to store the sizes of other buffers. Currently this is only
+// StringView and BinaryView.
+func HasBufferSizesBuffer(id arrow.Type) bool {
+	switch id {
+	case arrow.STRING_VIEW:
+		return true
+	case arrow.BINARY_VIEW:
+		return true

Review Comment:
   ```go
   switch id {
   case arrow.STRING_VIEW, arrow.BINARY_VIEW:
           return true
   ```



##########
go/arrow/type_traits.go:
##########
@@ -0,0 +1,147 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package arrow
+
+import (
+	"reflect"
+	"unsafe"
+
+	"github.com/apache/arrow/go/v15/arrow/decimal128"
+	"github.com/apache/arrow/go/v15/arrow/decimal256"
+	"github.com/apache/arrow/go/v15/arrow/float16"
+	"golang.org/x/exp/constraints"
+)
+
+// IntType is a type constraint for raw values represented as signed
+// integer types by  We aren't just using constraints.Signed
+// because we don't want to include the raw `int` type here whose size
+// changes based on the architecture (int32 on 32-bit architectures and
+// int64 on 64-bit architectures).
+//
+// This will also cover types like MonthInterval or the time types
+// as their underlying types are int32 and int64 which will get covered
+// by using the ~
+type IntType interface {
+	~int8 | ~int16 | ~int32 | ~int64
+}
+
+// UintType is a type constraint for raw values represented as unsigned
+// integer types by  We aren't just using constraints.Unsigned
+// because we don't want to include the raw `uint` type here whose size
+// changes based on the architecture (uint32 on 32-bit architectures and
+// uint64 on 64-bit architectures). We also don't want to include uintptr
+type UintType interface {
+	~uint8 | ~uint16 | ~uint32 | ~uint64
+}
+
+// FloatType is a type constraint for raw values for representing
+// floating point values in  This consists of constraints.Float and
+// float16.Num
+type FloatType interface {
+	float16.Num | constraints.Float
+}
+
+// NumericType is a type constraint for just signed/unsigned integers
+// and float32/float64.
+type NumericType interface {
+	IntType | UintType | constraints.Float
+}
+
+// FixedWidthType is a type constraint for raw values in Arrow that
+// can be represented as FixedWidth byte slices. Specifically this is for
+// using Go generics to easily re-type a byte slice to a properly-typed
+// slice. Booleans are excluded here since they are represented by Arrow
+// as a bitmap and thus the buffer can't be just reinterpreted as a []bool
+type FixedWidthType interface {
+	IntType | UintType |
+		FloatType | decimal128.Num | decimal256.Num |
+		DayTimeInterval | MonthDayNanoInterval
+}
+
+type TemporalType interface {
+	Date32 | Date64 | Time32 | Time64 |
+		Timestamp | Duration | DayTimeInterval |
+		MonthInterval | MonthDayNanoInterval
+}
+
+func sliceAs[Out, T interface{}](b []T) []Out {
+  len_bytes := len(b) * int(unsafe.Sizeof(b[0]))
+  cap_bytes := cap(b) * int(unsafe.Sizeof(b[0]))
+
+  var z Out
+  len_out := len_bytes / int(unsafe.Sizeof(z))
+  cap_out := cap_bytes / int(unsafe.Sizeof(z))
+
+	h := (*reflect.SliceHeader)(unsafe.Pointer(&b))
+  return unsafe.Slice((*Out)(unsafe.Pointer(h.Data)), cap_out)[:len_out]
+}
+
+func GetValues[T FixedWidthType](data ArrayData, i int) []T {
+	if data.Buffers()[i] == nil || data.Buffers()[i].Len() == 0 {
+		return nil
+	}
+	return sliceAs[T](data.Buffers()[i].Bytes())[data.Offset():data.Offset()+data.Len()]
+}
+
+func GetOffsets[T int32 | int64](data ArrayData, i int) []T {

Review Comment:
   same here and the ones below it



##########
go/arrow/type_traits.go:
##########
@@ -0,0 +1,147 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package arrow
+
+import (
+	"reflect"
+	"unsafe"
+
+	"github.com/apache/arrow/go/v15/arrow/decimal128"
+	"github.com/apache/arrow/go/v15/arrow/decimal256"
+	"github.com/apache/arrow/go/v15/arrow/float16"
+	"golang.org/x/exp/constraints"
+)
+
+// IntType is a type constraint for raw values represented as signed
+// integer types by  We aren't just using constraints.Signed
+// because we don't want to include the raw `int` type here whose size
+// changes based on the architecture (int32 on 32-bit architectures and
+// int64 on 64-bit architectures).
+//
+// This will also cover types like MonthInterval or the time types
+// as their underlying types are int32 and int64 which will get covered
+// by using the ~
+type IntType interface {
+	~int8 | ~int16 | ~int32 | ~int64
+}
+
+// UintType is a type constraint for raw values represented as unsigned
+// integer types by  We aren't just using constraints.Unsigned
+// because we don't want to include the raw `uint` type here whose size
+// changes based on the architecture (uint32 on 32-bit architectures and
+// uint64 on 64-bit architectures). We also don't want to include uintptr
+type UintType interface {
+	~uint8 | ~uint16 | ~uint32 | ~uint64
+}
+
+// FloatType is a type constraint for raw values for representing
+// floating point values in  This consists of constraints.Float and
+// float16.Num
+type FloatType interface {
+	float16.Num | constraints.Float
+}
+
+// NumericType is a type constraint for just signed/unsigned integers
+// and float32/float64.
+type NumericType interface {
+	IntType | UintType | constraints.Float
+}
+
+// FixedWidthType is a type constraint for raw values in Arrow that
+// can be represented as FixedWidth byte slices. Specifically this is for
+// using Go generics to easily re-type a byte slice to a properly-typed
+// slice. Booleans are excluded here since they are represented by Arrow
+// as a bitmap and thus the buffer can't be just reinterpreted as a []bool
+type FixedWidthType interface {
+	IntType | UintType |
+		FloatType | decimal128.Num | decimal256.Num |
+		DayTimeInterval | MonthDayNanoInterval
+}
+
+type TemporalType interface {
+	Date32 | Date64 | Time32 | Time64 |
+		Timestamp | Duration | DayTimeInterval |
+		MonthInterval | MonthDayNanoInterval
+}
+
+func sliceAs[Out, T interface{}](b []T) []Out {
+  len_bytes := len(b) * int(unsafe.Sizeof(b[0]))
+  cap_bytes := cap(b) * int(unsafe.Sizeof(b[0]))
+
+  var z Out
+  len_out := len_bytes / int(unsafe.Sizeof(z))
+  cap_out := cap_bytes / int(unsafe.Sizeof(z))
+
+	h := (*reflect.SliceHeader)(unsafe.Pointer(&b))
+  return unsafe.Slice((*Out)(unsafe.Pointer(h.Data)), cap_out)[:len_out]

Review Comment:
   fix the indentation, and let's make this more portable by just doing `unsafe.Pointer(&b[0])` instead of going to the `reflect.SliceHeader`



##########
go/arrow/type_traits.go:
##########
@@ -0,0 +1,147 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package arrow
+
+import (
+	"reflect"
+	"unsafe"
+
+	"github.com/apache/arrow/go/v15/arrow/decimal128"
+	"github.com/apache/arrow/go/v15/arrow/decimal256"
+	"github.com/apache/arrow/go/v15/arrow/float16"
+	"golang.org/x/exp/constraints"
+)
+
+// IntType is a type constraint for raw values represented as signed
+// integer types by  We aren't just using constraints.Signed
+// because we don't want to include the raw `int` type here whose size
+// changes based on the architecture (int32 on 32-bit architectures and
+// int64 on 64-bit architectures).
+//
+// This will also cover types like MonthInterval or the time types
+// as their underlying types are int32 and int64 which will get covered
+// by using the ~
+type IntType interface {
+	~int8 | ~int16 | ~int32 | ~int64
+}
+
+// UintType is a type constraint for raw values represented as unsigned
+// integer types by  We aren't just using constraints.Unsigned
+// because we don't want to include the raw `uint` type here whose size
+// changes based on the architecture (uint32 on 32-bit architectures and
+// uint64 on 64-bit architectures). We also don't want to include uintptr
+type UintType interface {
+	~uint8 | ~uint16 | ~uint32 | ~uint64
+}
+
+// FloatType is a type constraint for raw values for representing
+// floating point values in  This consists of constraints.Float and
+// float16.Num
+type FloatType interface {
+	float16.Num | constraints.Float
+}
+
+// NumericType is a type constraint for just signed/unsigned integers
+// and float32/float64.
+type NumericType interface {
+	IntType | UintType | constraints.Float
+}
+
+// FixedWidthType is a type constraint for raw values in Arrow that
+// can be represented as FixedWidth byte slices. Specifically this is for
+// using Go generics to easily re-type a byte slice to a properly-typed
+// slice. Booleans are excluded here since they are represented by Arrow
+// as a bitmap and thus the buffer can't be just reinterpreted as a []bool
+type FixedWidthType interface {
+	IntType | UintType |
+		FloatType | decimal128.Num | decimal256.Num |
+		DayTimeInterval | MonthDayNanoInterval
+}
+
+type TemporalType interface {
+	Date32 | Date64 | Time32 | Time64 |
+		Timestamp | Duration | DayTimeInterval |
+		MonthInterval | MonthDayNanoInterval
+}
+
+func sliceAs[Out, T interface{}](b []T) []Out {
+  len_bytes := len(b) * int(unsafe.Sizeof(b[0]))
+  cap_bytes := cap(b) * int(unsafe.Sizeof(b[0]))
+
+  var z Out
+  len_out := len_bytes / int(unsafe.Sizeof(z))
+  cap_out := cap_bytes / int(unsafe.Sizeof(z))
+
+	h := (*reflect.SliceHeader)(unsafe.Pointer(&b))
+  return unsafe.Slice((*Out)(unsafe.Pointer(h.Data)), cap_out)[:len_out]
+}
+
+func GetValues[T FixedWidthType](data ArrayData, i int) []T {

Review Comment:
   since we're exposing this now, we should add a proper comment docstring to it



##########
go/arrow/type_traits.go:
##########
@@ -0,0 +1,147 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package arrow
+
+import (
+	"reflect"
+	"unsafe"
+
+	"github.com/apache/arrow/go/v15/arrow/decimal128"
+	"github.com/apache/arrow/go/v15/arrow/decimal256"
+	"github.com/apache/arrow/go/v15/arrow/float16"
+	"golang.org/x/exp/constraints"
+)
+
+// IntType is a type constraint for raw values represented as signed
+// integer types by  We aren't just using constraints.Signed
+// because we don't want to include the raw `int` type here whose size
+// changes based on the architecture (int32 on 32-bit architectures and
+// int64 on 64-bit architectures).
+//
+// This will also cover types like MonthInterval or the time types
+// as their underlying types are int32 and int64 which will get covered
+// by using the ~
+type IntType interface {
+	~int8 | ~int16 | ~int32 | ~int64
+}
+
+// UintType is a type constraint for raw values represented as unsigned
+// integer types by  We aren't just using constraints.Unsigned
+// because we don't want to include the raw `uint` type here whose size
+// changes based on the architecture (uint32 on 32-bit architectures and
+// uint64 on 64-bit architectures). We also don't want to include uintptr
+type UintType interface {
+	~uint8 | ~uint16 | ~uint32 | ~uint64
+}
+
+// FloatType is a type constraint for raw values for representing
+// floating point values in  This consists of constraints.Float and
+// float16.Num
+type FloatType interface {
+	float16.Num | constraints.Float
+}
+
+// NumericType is a type constraint for just signed/unsigned integers
+// and float32/float64.
+type NumericType interface {
+	IntType | UintType | constraints.Float
+}
+
+// FixedWidthType is a type constraint for raw values in Arrow that
+// can be represented as FixedWidth byte slices. Specifically this is for
+// using Go generics to easily re-type a byte slice to a properly-typed
+// slice. Booleans are excluded here since they are represented by Arrow
+// as a bitmap and thus the buffer can't be just reinterpreted as a []bool
+type FixedWidthType interface {
+	IntType | UintType |
+		FloatType | decimal128.Num | decimal256.Num |
+		DayTimeInterval | MonthDayNanoInterval
+}
+
+type TemporalType interface {
+	Date32 | Date64 | Time32 | Time64 |
+		Timestamp | Duration | DayTimeInterval |
+		MonthInterval | MonthDayNanoInterval
+}
+
+func sliceAs[Out, T interface{}](b []T) []Out {
+  len_bytes := len(b) * int(unsafe.Sizeof(b[0]))
+  cap_bytes := cap(b) * int(unsafe.Sizeof(b[0]))

Review Comment:
   this'll blow up if `len(b) == 0`



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1428379845


##########
go/arrow/array/list.go:
##########
@@ -1411,118 +1410,19 @@ func (b *baseListViewBuilder) UnmarshalJSON(data []byte) error {
 	return b.Unmarshal(dec)
 }
 
-// Pre-conditions:
-//
-//	input.DataType() is ListViewType
-//	input.Len() > 0 && input.NullN() != input.Len()
-func minListViewOffset32(input arrow.ArrayData) int32 {
-	var bitmap []byte
-	if input.Buffers()[0] != nil {
-		bitmap = input.Buffers()[0].Bytes()
-	}
-	offsets := arrow.Int32Traits.CastFromBytes(input.Buffers()[1].Bytes())[input.Offset():]
-	sizes := arrow.Int32Traits.CastFromBytes(input.Buffers()[2].Bytes())[input.Offset():]
-
-	isNull := func(i int) bool {
-		return bitmap != nil && bitutil.BitIsNotSet(bitmap, input.Offset()+i)
-	}
-
-	// It's very likely that the first non-null non-empty list-view starts at
-	// offset 0 of the child array.
-	i := 0
-	for i < input.Len() && (isNull(i) || sizes[i] == 0) {
-		i += 1
-	}
-	if i >= input.Len() {
-		return 0
-	}
-	minOffset := offsets[i]
-	if minOffset == 0 {
-		// early exit: offset 0 found already
-		return 0
-	}
-
-	// Slow path: scan the buffers entirely.
-	i += 1
-	for ; i < input.Len(); i += 1 {
-		if isNull(i) {
-			continue
-		}
-		offset := offsets[i]
-		if offset < minOffset && sizes[i] > 0 {
-			minOffset = offset
-		}
-	}
-	return minOffset
-}
-
-// Find the maximum offset+size in a LIST_VIEW array.
+// Find the minimum offset+size in a LIST_VIEW/LARGE_LIST_VIEW array.
 //
 // Pre-conditions:
 //
-//	input.DataType() is ListViewType
-//	input.Len() > 0 && input.NullN() != input.Len()
-func maxListViewOffset32(input arrow.ArrayData) int {
-	inputOffset := input.Offset()
+//	input.DataType() is ListViewType if Offset=int32 or LargeListViewType if Offset=int64
+//	input.Len() > 0

Review Comment:
   ```suggestion
   //	input.Len() > 0 && input.NullN() != input.Len()
   ```



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1428033919


##########
dev/archery/archery/integration/datagen.py:
##########
@@ -927,6 +927,85 @@ class LargeListColumn(_BaseListColumn, _LargeOffsetsMixin):
     pass
 
 
+class ListViewField(Field):
+
+    def __init__(self, name, value_field, *, nullable=True,
+                 metadata=None):
+        super().__init__(name, nullable=nullable,
+                         metadata=metadata)
+        self.value_field = value_field
+
+    @property
+    def column_class(self):
+        return ListViewColumn
+
+    def _get_type(self):
+        return OrderedDict([
+            ('name', 'listview')
+        ])
+
+    def _get_children(self):
+        return [self.value_field.get_json()]
+
+    def generate_column(self, size, name=None):
+        MAX_LIST_SIZE = 4
+
+        is_valid = self._make_is_valid(size)
+        offsets = []
+        sizes = np.random.randint(0, MAX_LIST_SIZE + 1, size=size)
+        offset = 0
+        for s in sizes:
+            offsets.append(offset)
+            offset += int(s)

Review Comment:
   https://github.com/apache/arrow/pull/39019/commits/03a12f9cf50dbb92184afd815a73454b138ce713



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1427332140


##########
go/arrow/internal/debug/log_on.go:
##########
@@ -14,6 +14,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+//go:build debug

Review Comment:
   no, compiler directives like build constraints explicitly have no space after the `//`



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1425677518


##########
dev/archery/archery/integration/runner.py:
##########
@@ -193,6 +193,8 @@ def _run_test_cases(self,
         ``case_runner`` ran against ``test_cases``
         """
         def case_wrapper(test_case):
+            if serial:
+                return case_runner(test_case)

Review Comment:
   Is this addition deliberate?



##########
dev/archery/archery/integration/datagen.py:
##########
@@ -927,6 +927,85 @@ class LargeListColumn(_BaseListColumn, _LargeOffsetsMixin):
     pass
 
 
+class ListViewField(Field):
+
+    def __init__(self, name, value_field, *, nullable=True,
+                 metadata=None):
+        super().__init__(name, nullable=nullable,
+                         metadata=metadata)
+        self.value_field = value_field
+
+    @property
+    def column_class(self):
+        return ListViewColumn
+
+    def _get_type(self):
+        return OrderedDict([

Review Comment:
   FTR, we don't need `OrderedDict` anymore as regular dicts are now ordered by default.



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1428086058


##########
go/arrow/type_traits.go:
##########
@@ -0,0 +1,147 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package arrow
+
+import (
+	"reflect"
+	"unsafe"
+
+	"github.com/apache/arrow/go/v15/arrow/decimal128"
+	"github.com/apache/arrow/go/v15/arrow/decimal256"
+	"github.com/apache/arrow/go/v15/arrow/float16"
+	"golang.org/x/exp/constraints"
+)
+
+// IntType is a type constraint for raw values represented as signed
+// integer types by  We aren't just using constraints.Signed
+// because we don't want to include the raw `int` type here whose size
+// changes based on the architecture (int32 on 32-bit architectures and
+// int64 on 64-bit architectures).
+//
+// This will also cover types like MonthInterval or the time types
+// as their underlying types are int32 and int64 which will get covered
+// by using the ~
+type IntType interface {
+	~int8 | ~int16 | ~int32 | ~int64
+}
+
+// UintType is a type constraint for raw values represented as unsigned
+// integer types by  We aren't just using constraints.Unsigned
+// because we don't want to include the raw `uint` type here whose size
+// changes based on the architecture (uint32 on 32-bit architectures and
+// uint64 on 64-bit architectures). We also don't want to include uintptr
+type UintType interface {
+	~uint8 | ~uint16 | ~uint32 | ~uint64
+}
+
+// FloatType is a type constraint for raw values for representing
+// floating point values in  This consists of constraints.Float and
+// float16.Num
+type FloatType interface {
+	float16.Num | constraints.Float
+}
+
+// NumericType is a type constraint for just signed/unsigned integers
+// and float32/float64.
+type NumericType interface {
+	IntType | UintType | constraints.Float
+}
+
+// FixedWidthType is a type constraint for raw values in Arrow that
+// can be represented as FixedWidth byte slices. Specifically this is for
+// using Go generics to easily re-type a byte slice to a properly-typed
+// slice. Booleans are excluded here since they are represented by Arrow
+// as a bitmap and thus the buffer can't be just reinterpreted as a []bool
+type FixedWidthType interface {
+	IntType | UintType |
+		FloatType | decimal128.Num | decimal256.Num |
+		DayTimeInterval | MonthDayNanoInterval
+}
+
+type TemporalType interface {
+	Date32 | Date64 | Time32 | Time64 |
+		Timestamp | Duration | DayTimeInterval |
+		MonthInterval | MonthDayNanoInterval
+}
+
+func sliceAs[Out, T interface{}](b []T) []Out {
+  len_bytes := len(b) * int(unsafe.Sizeof(b[0]))
+  cap_bytes := cap(b) * int(unsafe.Sizeof(b[0]))
+
+  var z Out
+  len_out := len_bytes / int(unsafe.Sizeof(z))
+  cap_out := cap_bytes / int(unsafe.Sizeof(z))
+
+	h := (*reflect.SliceHeader)(unsafe.Pointer(&b))
+  return unsafe.Slice((*Out)(unsafe.Pointer(h.Data)), cap_out)[:len_out]

Review Comment:
   https://github.com/apache/arrow/pull/39019/commits/cb6a4efdd74c4b496572f2223be39f286bdcb5d1



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1427319054


##########
go/arrow/array/list.go:
##########
@@ -1411,217 +1410,56 @@ func (b *baseListViewBuilder) UnmarshalJSON(data []byte) error {
 	return b.Unmarshal(dec)
 }
 
-// Pre-conditions:
-//
-//	input.DataType() is ListViewType
-//	input.Len() > 0 && input.NullN() != input.Len()
-func minListViewOffset32(input arrow.ArrayData) int32 {
-	var bitmap []byte
-	if input.Buffers()[0] != nil {
-		bitmap = input.Buffers()[0].Bytes()
-	}
-	offsets := arrow.Int32Traits.CastFromBytes(input.Buffers()[1].Bytes())[input.Offset():]
-	sizes := arrow.Int32Traits.CastFromBytes(input.Buffers()[2].Bytes())[input.Offset():]
-
-	isNull := func(i int) bool {
-		return bitmap != nil && bitutil.BitIsNotSet(bitmap, input.Offset()+i)
-	}
-
-	// It's very likely that the first non-null non-empty list-view starts at
-	// offset 0 of the child array.
-	i := 0
-	for i < input.Len() && (isNull(i) || sizes[i] == 0) {
-		i += 1
-	}
-	if i >= input.Len() {
-		return 0
-	}
-	minOffset := offsets[i]
-	if minOffset == 0 {
-		// early exit: offset 0 found already
-		return 0
-	}
-
-	// Slow path: scan the buffers entirely.
-	i += 1
-	for ; i < input.Len(); i += 1 {
-		if isNull(i) {
-			continue
-		}
-		offset := offsets[i]
-		if offset < minOffset && sizes[i] > 0 {
-			minOffset = offset
-		}
-	}
-	return minOffset
-}
-
-// Find the maximum offset+size in a LIST_VIEW array.
+// Find the minimum offset+size in a LIST_VIEW/LARGE_LIST_VIEW array.
 //
 // Pre-conditions:
 //
-//	input.DataType() is ListViewType
-//	input.Len() > 0 && input.NullN() != input.Len()
-func maxListViewOffset32(input arrow.ArrayData) int {
+//	input.DataType() is ListViewType if Offset=int32 or LargeListViewType if Offset=int64
+//	input.Len() > 0

Review Comment:
   RangeOfUsedValues was causing an error in IPC writing where it was being used to determine how much of the values array to write. This could drop values referenced by views under null bits, which later produced negative offsets. I was trying to rewrite this function to also consider views under null bits (less efficient for concat but still usable for IPC writing). I have reverted the change to RangeOfUsedValues and IPC writing no longer tries to slice the values array. This is less optimal for IPC writing but we can add optimizations back later, for now   I'd like to get the integration test passing.



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1427302977


##########
go/arrow/cdata/cdata_exports.go:
##########
@@ -328,6 +336,15 @@ func allocateBufferPtrArr(n int) (out []*C.void) {
 	return
 }
 
+func allocateBufferSizeArr(n int) (out []C.int64_t) {
+	s := (*reflect.SliceHeader)(unsafe.Pointer(&out))
+	s.Data = uintptr(C.calloc(C.size_t(n), C.size_t(unsafe.Sizeof(int64(0)))))
+	s.Len = n
+	s.Cap = n
+
+	return

Review Comment:
   It is since there's a named return value



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1425745066


##########
go/arrow/cdata/cdata_exports.go:
##########
@@ -328,6 +336,15 @@ func allocateBufferPtrArr(n int) (out []*C.void) {
 	return
 }
 
+func allocateBufferSizeArr(n int) (out []C.int64_t) {
+	s := (*reflect.SliceHeader)(unsafe.Pointer(&out))
+	s.Data = uintptr(C.calloc(C.size_t(n), C.size_t(unsafe.Sizeof(int64(0)))))
+	s.Len = n
+	s.Cap = n
+
+	return

Review Comment:
   is this necessary?



##########
go/arrow/internal/flatbuf/DictionaryBatch.go:
##########
@@ -73,9 +73,9 @@ func (rcv *DictionaryBatch) Data(obj *RecordBatch) *RecordBatch {
 	return nil
 }
 
-/// If isDelta is true the values in the dictionary are to be appended to a
-/// dictionary with the indicated id. If isDelta is false this dictionary
-/// should replace the existing dictionary.
+// / If isDelta is true the values in the dictionary are to be appended to a
+// / dictionary with the indicated id. If isDelta is false this dictionary
+// / should replace the existing dictionary.

Review Comment:
   I think you should undo these changes `git checkout -p origin/main` and have a separate PR where we make the who Go codebase auto-formattable.



##########
go/arrow/compute/fieldref.go:
##########
@@ -346,17 +346,18 @@ func FieldRefList(elems ...interface{}) FieldRef {
 // NewFieldRefFromDotPath parses a dot path into a field ref.
 //
 // dot_path = '.' name
-//			| '[' digit+ ']'
-//			| dot_path+
+//
+//	| '[' digit+ ']'
+//	| dot_path+
 //
 // Examples
 //
-// 		".alpha" => FieldRefName("alpha")
-//		"[2]" => FieldRefIndex(2)
-//		".beta[3]" => FieldRefList("beta", 3)
-//		"[5].gamma.delta[7]" => FieldRefList(5, "gamma", "delta", 7)
-//		".hello world" => FieldRefName("hello world")
-//		`.\[y\]\\tho\.\` => FieldRef(`[y]\tho.\`)
+//	".alpha" => FieldRefName("alpha")
+//	"[2]" => FieldRefIndex(2)
+//	".beta[3]" => FieldRefList("beta", 3)
+//	"[5].gamma.delta[7]" => FieldRefList(5, "gamma", "delta", 7)
+//	".hello world" => FieldRefName("hello world")
+//	`.\[y\]\\tho\.\` => FieldRef(`[y]\tho.\`)

Review Comment:
   Code should be indented or start end and with ` ``` `



##########
go/arrow/internal/flatbuf/Binary.go:
##########
@@ -22,7 +22,7 @@ import (
 	flatbuffers "github.com/google/flatbuffers/go"
 )
 
-/// Opaque binary data
+// / Opaque binary data

Review Comment:
   Auto formatter noise? How can we prevent this?



##########
go/arrow/internal/debug/log_on.go:
##########
@@ -14,6 +14,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+//go:build debug

Review Comment:
   no space after `//`?



##########
go/arrow/compute/fieldref.go:
##########
@@ -346,17 +346,18 @@ func FieldRefList(elems ...interface{}) FieldRef {
 // NewFieldRefFromDotPath parses a dot path into a field ref.
 //
 // dot_path = '.' name
-//			| '[' digit+ ']'
-//			| dot_path+
+//
+//	| '[' digit+ ']'
+//	| dot_path+

Review Comment:
   the `|` should align with the `=` on the previous line



##########
go/arrow/cdata/cdata.go:
##########
@@ -683,6 +743,43 @@ func (imp *cimporter) importListLike() (err error) {
 	return
 }
 
+func (imp *cimporter) importListViewLike() (err error) {
+	offsetSize := int64(imp.dt.Layout().Buffers[1].ByteWidth)
+
+	if err = imp.checkNumChildren(1); err != nil {
+		return err
+	}
+
+	if err = imp.checkNumBuffers(3); err != nil {
+		return err
+	}
+
+	var nulls, offsets, sizes *memory.Buffer
+	if nulls, err = imp.importNullBitmap(0); err != nil {
+		return
+	}
+	if nulls != nil {
+		defer nulls.Release()
+	}
+
+	if offsets, err = imp.importOffsetsBuffer(1, offsetSize); err != nil {

Review Comment:
   Is `importOffsetsBuffer` importing only `len` offsets instead of `len + 1`?



##########
go/arrow/compute/fieldref.go:
##########
@@ -282,31 +282,31 @@ type refImpl interface {
 //
 // Nested fields can be referenced as well, given the schema:
 //
-// 		arrow.NewSchema([]arrow.Field{
-//			{Name: "a", Type: arrow.StructOf(arrow.Field{Name: "n", Type: arrow.Null})},
-//  		{Name: "b", Type: arrow.PrimitiveTypes.Int32},
-// 		})
+//			arrow.NewSchema([]arrow.Field{
+//				{Name: "a", Type: arrow.StructOf(arrow.Field{Name: "n", Type: arrow.Null})},
+//	 		{Name: "b", Type: arrow.PrimitiveTypes.Int32},
+//			})
 //
 // the following all indicate the nested field named "n":
 //
-//		FieldRefPath(FieldPath{0, 0})
-//		FieldRefList("a", 0)
-// 		FieldRefList("a", "n")
-// 		FieldRefList(0, "n")
-//		NewFieldRefFromDotPath(".a[0]")
+//	FieldRefPath(FieldPath{0, 0})
+//	FieldRefList("a", 0)
+//	FieldRefList("a", "n")
+//	FieldRefList(0, "n")
+//	NewFieldRefFromDotPath(".a[0]")
 //
 // FieldPaths matching a FieldRef are retrieved with the FindAll* functions
 // Multiple matches are possible because field names may be duplicated within
 // a schema. For example:
 //
-//		aIsAmbiguous := arrow.NewSchema([]arrow.Field{
-//			{Name: "a", Type: arrow.PrimitiveTypes.Int32},
-//			{Name: "a", Type: arrow.PrimitiveTypes.Float32},
-//		})
-//		matches := FieldRefName("a").FindAll(aIsAmbiguous)
-//		assert.Len(matches, 2)
-//		assert.True(matches[0].Get(aIsAmbiguous).Equals(aIsAmbiguous.Field(0))
-//		assert.True(matches[1].Get(aIsAmbiguous).Equals(aIsAmbiguous.Field(1))
+//	aIsAmbiguous := arrow.NewSchema([]arrow.Field{
+//		{Name: "a", Type: arrow.PrimitiveTypes.Int32},
+//		{Name: "a", Type: arrow.PrimitiveTypes.Float32},
+//	})
+//	matches := FieldRefName("a").FindAll(aIsAmbiguous)
+//	assert.Len(matches, 2)
+//	assert.True(matches[0].Get(aIsAmbiguous).Equals(aIsAmbiguous.Field(0))
+//	assert.True(matches[1].Get(aIsAmbiguous).Equals(aIsAmbiguous.Field(1))

Review Comment:
   If this is markdown, code should be indented 4 spaces.



##########
go/arrow/compute/fieldref.go:
##########
@@ -282,31 +282,31 @@ type refImpl interface {
 //
 // Nested fields can be referenced as well, given the schema:
 //
-// 		arrow.NewSchema([]arrow.Field{
-//			{Name: "a", Type: arrow.StructOf(arrow.Field{Name: "n", Type: arrow.Null})},
-//  		{Name: "b", Type: arrow.PrimitiveTypes.Int32},
-// 		})
+//			arrow.NewSchema([]arrow.Field{
+//				{Name: "a", Type: arrow.StructOf(arrow.Field{Name: "n", Type: arrow.Null})},
+//	 		{Name: "b", Type: arrow.PrimitiveTypes.Int32},
+//			})

Review Comment:
   Why the indentation increased here?



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1427304463


##########
go/arrow/cdata/cdata.go:
##########
@@ -683,6 +743,43 @@ func (imp *cimporter) importListLike() (err error) {
 	return
 }
 
+func (imp *cimporter) importListViewLike() (err error) {
+	offsetSize := int64(imp.dt.Layout().Buffers[1].ByteWidth)
+
+	if err = imp.checkNumChildren(1); err != nil {
+		return err
+	}
+
+	if err = imp.checkNumBuffers(3); err != nil {
+		return err
+	}
+
+	var nulls, offsets, sizes *memory.Buffer
+	if nulls, err = imp.importNullBitmap(0); err != nil {
+		return
+	}
+	if nulls != nil {
+		defer nulls.Release()
+	}
+
+	if offsets, err = imp.importOffsetsBuffer(1, offsetSize); err != nil {

Review Comment:
   Nope, will replace with importFixedSizeBuffer



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #39019:
URL: https://github.com/apache/arrow/pull/39019#discussion_r1428033635


##########
go/arrow/internal/arrjson/arrjson.go:
##########
@@ -1090,7 +1091,7 @@ func arrayFromJSON(mem memory.Allocator, dt arrow.DataType, arr Array) arrow.Arr
 	case arrow.BinaryViewDataType:
 		valids := validsToBitmap(validsFromJSON(arr.Valids), mem)
 		nulls := arr.Count - bitutil.CountSetBits(valids.Bytes(), 0, arr.Count)
-		headers := stringHeadersFromJSON(mem, !dt.IsUtf8(), arr.Data)
+		headers := stringHeadersFromJSON(mem, !dt.IsUtf8(), arr.Views)

Review Comment:
   https://github.com/apache/arrow/pull/39019/commits/b9237669d321e38bdeeffb1e675be5399a9575e8



-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #39019:
URL: https://github.com/apache/arrow/pull/39019#issuecomment-1860813736

   if there's no objections or no other changes requested I'll merge this at the end of the day


-- 
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


Re: [PR] GH-39013: [Go][Integration] Support cABI import/export of StringView [arrow]

Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #39019:
URL: https://github.com/apache/arrow/pull/39019


-- 
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