Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Go][Parquet] Improve the MapOf and ListOf helpers #36696

Closed
wolfeidau opened this issue Jul 14, 2023 · 0 comments · Fixed by #36697
Closed

[Go][Parquet] Improve the MapOf and ListOf helpers #36696

wolfeidau opened this issue Jul 14, 2023 · 0 comments · Fixed by #36697

Comments

@wolfeidau
Copy link
Contributor

Describe the enhancement requested

Currently the error messages the map and list helpers provide aren't super clear on why things are rejected and refer to a panic which doesn't happen.

Would be great if these methods where reviewed and updated.

Component(s)

Go

wolfeidau added a commit to wolfeidau/arrow that referenced this issue Jul 14, 2023
* Removed references to panics I can't find
* Updated error messages for list and map to be clearer with validation errors
* Added a ListOfWithName to provide a clearer matching method to MapOf which takes a name
wolfeidau added a commit to wolfeidau/arrow that referenced this issue Jul 14, 2023
* Removed references to panics I can't find
* Updated error messages for list and map to be clearer with validation errors
* Added a ListOfWithName to provide a clearer matching method to MapOf which takes a name
zeroshade pushed a commit that referenced this issue Jul 20, 2023
### Rationale for this change

The aim is to improve the MapOf and ListOf helper functions without breaking anything. I have added a `ListOfWithName` which matches the `MapOf` function in that it takes a name, rather than deriving it from the elements name, which should actually be `element`.

This just seems clearer to me as an interface, and makes construction a bit more obvious.

### What changes are included in this PR?

* Removed references to panics I can't find
* Updated error messages for list and map to be clearer with validation errors
* Added a ListOfWithName to provide a clearer matching method to MapOf which takes a name

Closes #36696 

### Are these changes tested?

Yes, I added a test for the new `ListOfWithName` function.

### Are there any user-facing changes?

* Closes: #36696

Authored-by: Mark Wolfe <mark@wolfe.id.au>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
@zeroshade zeroshade added this to the 14.0.0 milestone Jul 20, 2023
R-JunmingChen pushed a commit to R-JunmingChen/arrow that referenced this issue Aug 20, 2023
### Rationale for this change

The aim is to improve the MapOf and ListOf helper functions without breaking anything. I have added a `ListOfWithName` which matches the `MapOf` function in that it takes a name, rather than deriving it from the elements name, which should actually be `element`.

This just seems clearer to me as an interface, and makes construction a bit more obvious.

### What changes are included in this PR?

* Removed references to panics I can't find
* Updated error messages for list and map to be clearer with validation errors
* Added a ListOfWithName to provide a clearer matching method to MapOf which takes a name

Closes apache#36696 

### Are these changes tested?

Yes, I added a test for the new `ListOfWithName` function.

### Are there any user-facing changes?

* Closes: apache#36696

Authored-by: Mark Wolfe <mark@wolfe.id.au>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
### Rationale for this change

The aim is to improve the MapOf and ListOf helper functions without breaking anything. I have added a `ListOfWithName` which matches the `MapOf` function in that it takes a name, rather than deriving it from the elements name, which should actually be `element`.

This just seems clearer to me as an interface, and makes construction a bit more obvious.

### What changes are included in this PR?

* Removed references to panics I can't find
* Updated error messages for list and map to be clearer with validation errors
* Added a ListOfWithName to provide a clearer matching method to MapOf which takes a name

Closes apache#36696 

### Are these changes tested?

Yes, I added a test for the new `ListOfWithName` function.

### Are there any user-facing changes?

* Closes: apache#36696

Authored-by: Mark Wolfe <mark@wolfe.id.au>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants