You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "lidavidm (via GitHub)" <gi...@apache.org> on 2023/06/09 18:25:41 UTC
[GitHub] [arrow] lidavidm opened a new pull request, #36015: GH-36014: [Go] Allow duplicate field names in structs
lidavidm opened a new pull request, #36015:
URL: https://github.com/apache/arrow/pull/36015
<!--
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 package segfaulted before.
### What changes are included in this PR?
Allow duplicate field names (in general methods will return the first field if there are duplicates)
### Are these changes tested?
Yes
### Are there any user-facing changes?
Yes
<!--
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] lidavidm commented on pull request #36015: GH-36014: [Go] Allow duplicate field names in structs
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #36015:
URL: https://github.com/apache/arrow/pull/36015#issuecomment-1587582371
Oh! I'll fix that, one moment
--
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] lidavidm commented on a diff in pull request #36015: GH-36014: [Go] Allow duplicate field names in structs
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #36015:
URL: https://github.com/apache/arrow/pull/36015#discussion_r1224763143
##########
go/arrow/datatype_nested.go:
##########
@@ -309,17 +309,28 @@ func (t *StructType) Fields() []Field {
func (t *StructType) Field(i int) Field { return t.fields[i] }
+// FieldByName gets the field with the given name.
+//
+// If there are multiple fields with the given name, FieldByName
+// returns the first such field.
func (t *StructType) FieldByName(name string) (Field, bool) {
i, ok := t.index[name]
if !ok {
return Field{}, false
}
- return t.fields[i], true
+ return t.fields[i[0]], true
}
+// FieldIdx gets the index of the field with the given name.
+//
+// If there are multiple fields with the given name, FieldIdx returns
+// the index of the first first such field.
func (t *StructType) FieldIdx(name string) (int, bool) {
i, ok := t.index[name]
- return i, ok
+ if ok {
+ return i[0], true
+ }
+ return -1, false
}
Review Comment:
Added the same methods Schema has, though the inconsistency admittedly bugs me (why is FieldsByName ([]Field, bool) but FieldIndices is just []int, and then there's FieldIndices vs FieldIdx -.-)
--
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] lidavidm merged pull request #36015: GH-36014: [Go] Allow duplicate field names in structs
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #36015:
URL: https://github.com/apache/arrow/pull/36015
--
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 #36015: GH-36014: [Go] Allow duplicate field names in structs
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #36015:
URL: https://github.com/apache/arrow/pull/36015#discussion_r1226784119
##########
go/arrow/datatype_nested.go:
##########
@@ -309,17 +309,28 @@ func (t *StructType) Fields() []Field {
func (t *StructType) Field(i int) Field { return t.fields[i] }
+// FieldByName gets the field with the given name.
+//
+// If there are multiple fields with the given name, FieldByName
+// returns the first such field.
func (t *StructType) FieldByName(name string) (Field, bool) {
i, ok := t.index[name]
if !ok {
return Field{}, false
}
- return t.fields[i], true
+ return t.fields[i[0]], true
}
+// FieldIdx gets the index of the field with the given name.
+//
+// If there are multiple fields with the given name, FieldIdx returns
+// the index of the first first such field.
func (t *StructType) FieldIdx(name string) (int, bool) {
i, ok := t.index[name]
- return i, ok
+ if ok {
+ return i[0], true
+ }
+ return -1, false
}
Review Comment:
Blame InfluxDB who created the original impl :stuck_out_tongue: lol
--
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 #36015: GH-36014: [Go] Allow duplicate field names in structs
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #36015:
URL: https://github.com/apache/arrow/pull/36015#issuecomment-1587489948
@lidavidm Actually, with this, can you remove the `skip_category('Go')` from archery/datagen.py for the `generate_duplicate_fieldnames_case()`? Or do you want to enable the integration test as a separate PR?
--
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 #36015: GH-36014: [Go] Allow duplicate field names in structs
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #36015:
URL: https://github.com/apache/arrow/pull/36015#issuecomment-1587688417
LGTM!
--
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 #36015: GH-36014: [Go] Allow duplicate field names in structs
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #36015:
URL: https://github.com/apache/arrow/pull/36015#issuecomment-1588170508
Benchmark runs are scheduled for baseline = a4d241c2f7da1ae11a309300a85cc25bc9ec107e and contender = b642707f6de72ec12c1d4a802680b0a3fdc6a216. b642707f6de72ec12c1d4a802680b0a3fdc6a216 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:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/9d136689d4d943d2b30689103b1da442...87e6988b0e464c1cb1759306052c0974/)
[Finished :arrow_down:1.27% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/fe595a799770475da914b49259ee6363...9ead9702810647118c7609be590b6272/)
[Finished :arrow_down:0.0% :arrow_up:0.65%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/16e992c1b4bf43149a101260e42f2725...4842a9c0c97f47c896967a1390bc9074/)
[Finished :arrow_down:0.89% :arrow_up:0.18%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/375d29f0990548338bb669fd31430772...cc6d8dcc1d2340a3a285f269c6f8422e/)
Buildkite builds:
[Finished] [`b642707f` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/3025)
[Finished] [`b642707f` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/3061)
[Finished] [`b642707f` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/3026)
[Finished] [`b642707f` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/3051)
[Finished] [`a4d241c2` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/3024)
[Finished] [`a4d241c2` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/3060)
[Finished] [`a4d241c2` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/3025)
[Finished] [`a4d241c2` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/3050)
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] zeroshade commented on a diff in pull request #36015: GH-36014: [Go] Allow duplicate field names in structs
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #36015:
URL: https://github.com/apache/arrow/pull/36015#discussion_r1224669623
##########
go/arrow/datatype_nested.go:
##########
@@ -309,17 +309,28 @@ func (t *StructType) Fields() []Field {
func (t *StructType) Field(i int) Field { return t.fields[i] }
+// FieldByName gets the field with the given name.
+//
+// If there are multiple fields with the given name, FieldByName
+// returns the first such field.
func (t *StructType) FieldByName(name string) (Field, bool) {
i, ok := t.index[name]
if !ok {
return Field{}, false
}
- return t.fields[i], true
+ return t.fields[i[0]], true
}
+// FieldIdx gets the index of the field with the given name.
+//
+// If there are multiple fields with the given name, FieldIdx returns
+// the index of the first first such field.
func (t *StructType) FieldIdx(name string) (int, bool) {
i, ok := t.index[name]
- return i, ok
+ if ok {
+ return i[0], true
+ }
+ return -1, false
}
Review Comment:
We should add a new method that will return the []int by name lookup
--
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] lidavidm commented on pull request #36015: GH-36014: [Go] Allow duplicate field names in structs
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #36015:
URL: https://github.com/apache/arrow/pull/36015#issuecomment-1587665470
Looks like it passes.
--
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