You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "candiduslynx (via GitHub)" <gi...@apache.org> on 2023/06/01 08:52:26 UTC
[GitHub] [arrow] candiduslynx opened a new pull request, #35867: GH-35866: [Go] Provide a copy in `arrow.NestedType.Fields()` implementations
candiduslynx opened a new pull request, #35867:
URL: https://github.com/apache/arrow/pull/35867
### Rationale for this change
Data types should be left immutable after they've been constructed.
### What changes are included in this PR?
Provide a copy of fields in the following implementations:
* `*arrow.StructType.Fields()`
* `*arrow.unionType.Fields()`
### Are these changes tested?
`arrow.TestFieldsImmutability` was added to ensure the behavior.
### Are there any user-facing changes?
Now the fields for structs & unions will be immutable.
<!--
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
[GitHub] [arrow] zeroshade merged pull request #35867: GH-35866: [Go] Provide a copy in `arrow.NestedType.Fields()` implementations
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #35867:
URL: https://github.com/apache/arrow/pull/35867
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #35867: GH-35866: [Go] Provide a copy in `arrow.NestedType.Fields()` implementations
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35867:
URL: https://github.com/apache/arrow/pull/35867#issuecomment-1571627037
:warning: GitHub issue #35866 **has been automatically assigned in GitHub** to PR creator.
--
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
[GitHub] [arrow] zeroshade commented on a diff in pull request #35867: GH-35866: [Go] Provide a copy in `arrow.NestedType.Fields()` implementations
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #35867:
URL: https://github.com/apache/arrow/pull/35867#discussion_r1213315826
##########
go/arrow/datatype_nested.go:
##########
@@ -490,7 +495,12 @@ func (t *unionType) init(fields []Field, typeCodes []UnionTypeCode) {
}
}
-func (t unionType) Fields() []Field { return t.children }
+func (t unionType) Fields() []Field {
+ fields := make([]Field, len(t.children))
+ copy(fields, t.children)
+ return fields
+}
Review Comment:
same here. add a godoc comment to this to indicate it makes a copy of the fields
##########
go/arrow/datatype_nested.go:
##########
@@ -288,7 +288,12 @@ func (t *StructType) String() string {
return o.String()
}
-func (t *StructType) Fields() []Field { return t.fields }
+func (t *StructType) Fields() []Field {
Review Comment:
Can you add a godoc comment on the methods to indicate to callers that this will make a copy of the Fields?
--
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
[GitHub] [arrow] zeroshade commented on pull request #35867: GH-35866: [Go] Provide a copy in `arrow.NestedType.Fields()` implementations
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #35867:
URL: https://github.com/apache/arrow/pull/35867#issuecomment-1572308959
@candiduslynx true, (a mistake in my wording) i meant if they didn't want to make a copy of the entire slice of fields. Copying an individual field is extremely cheap, but under certain circumstances it could be desireable to avoid copying the entire slice of fields.
Either way most times that a slice is returned from a method in go, the average usage assumes that it isn't making a copy of all the elements so it's good to ensure the godoc is clear.
--
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
[GitHub] [arrow] candiduslynx commented on a diff in pull request #35867: GH-35866: [Go] Provide a copy in `arrow.NestedType.Fields()` implementations
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on code in PR #35867:
URL: https://github.com/apache/arrow/pull/35867#discussion_r1213345109
##########
go/arrow/datatype_nested.go:
##########
@@ -288,7 +288,12 @@ func (t *StructType) String() string {
return o.String()
}
-func (t *StructType) Fields() []Field { return t.fields }
+func (t *StructType) Fields() []Field {
Review Comment:
1e81c723eb665da8f59519710c749e0fda435676
##########
go/arrow/datatype_nested.go:
##########
@@ -490,7 +495,12 @@ func (t *unionType) init(fields []Field, typeCodes []UnionTypeCode) {
}
}
-func (t unionType) Fields() []Field { return t.children }
+func (t unionType) Fields() []Field {
+ fields := make([]Field, len(t.children))
+ copy(fields, t.children)
+ return fields
+}
Review Comment:
1e81c723eb665da8f59519710c749e0fda435676
--
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
[GitHub] [arrow] ursabot commented on pull request #35867: GH-35866: [Go] Provide a copy in `arrow.NestedType.Fields()` implementations
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #35867:
URL: https://github.com/apache/arrow/pull/35867#issuecomment-1573752889
Benchmark runs are scheduled for baseline = c67fb399078b1cc1b4a10d88811f1c758ca91f82 and contender = e0fd7ef8f16a913913bba706c396f424fba1583a. e0fd7ef8f16a913913bba706c396f424fba1583a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:25.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/341ad88d1839448b90c7f5c308dfa3a7...357d2fba2ae647b2a7764e7e6caeb91b/)
[Failed :arrow_down:0.83% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/0a46ac1c92214e49888a9870d04d8e3f...d3adc704214844149419cb2e3154a327/)
[Finished :arrow_down:1.63% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/7f66fc2b62e346c39237dd59ec2e2a03...ad981000c2f54fa4bf34f42ea37e41ed/)
[Finished :arrow_down:0.39% :arrow_up:0.03%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/0687a523c76e40328b1a62d8d277f87b...831458cedb5a4e7fbdf9d2ff777d9036/)
Buildkite builds:
[Finished] [`e0fd7ef8` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2967)
[Finished] [`e0fd7ef8` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/3003)
[Finished] [`e0fd7ef8` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2968)
[Finished] [`e0fd7ef8` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2993)
[Finished] [`c67fb399` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2966)
[Failed] [`c67fb399` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/3002)
[Finished] [`c67fb399` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2967)
[Finished] [`c67fb399` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2992)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] candiduslynx commented on pull request #35867: GH-35866: [Go] Provide a copy in `arrow.NestedType.Fields()` implementations
Posted by "candiduslynx (via GitHub)" <gi...@apache.org>.
candiduslynx commented on PR #35867:
URL: https://github.com/apache/arrow/pull/35867#issuecomment-1572271997
> LGTM just want to have the documentation indicate that it makes a copy so that callers can choose to instead use the `Field(i)` methods if they want to avoid the copies
Just FYI, `Field(i)` provides a copy of the `arrow.Field` (note: the `Type` field will point to the same instance, but the pointer itself is copied)
--
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