Skip to content

Conversation

@zhongyujiang
Copy link
Contributor

#3605 (comment)
The RemoveIds won't keep the original custom name of repeatedElement of list/map type passed to it, instead uses a default name in new Parquet version. This PR fixed it.@rdblue can you help review this pr? Thanks!

@rdblue
Copy link
Contributor

rdblue commented Dec 20, 2021

@zhongyujiang, what is the problem caused by this?

@kbendick
Copy link
Contributor

@zhongyujiang could you update the PR description to further clarify what problem this is solving? It's hard to to tell when it's linked via a comment if we weren't involved in the initial comment discussion.

@rdblue
Copy link
Contributor

rdblue commented Dec 21, 2021

This is only called from test code, so I think the motivation was to speculative since other classes had similar updates. I don't think that we should move forward with this since it isn't actually used.

What you could do instead is move this into src/test/java.

@rdblue rdblue closed this Dec 21, 2021
@zhongyujiang
Copy link
Contributor Author

This is only called from test code, so I think the motivation was to speculative since other classes had similar updates. I don't think that we should move forward with this since it isn't actually used.

I didn't notice that this is only used in test, and no, I am not speculating. If you read this issue and this pr, you can find that I found the problem of ApplyNameMapping change custom parquet schema before another merged pr, I checked other classes extend ParquetTypeVisitor and found RemoveIds has the same problem. I fixed both in this pr, but no one reviewed. Yesterday I found the issue of ApplyNameMapping is solved in another pr and already merged, so I closed my last pr and extracted the part of RemoveIds here. That's my motivation.

@zhongyujiang zhongyujiang deleted the fix-removeids-change-type-name branch April 26, 2022 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants