Skip to content

Conversation

@rdblue
Copy link
Contributor

@rdblue rdblue commented Dec 19, 2021

This makes a couple of modifications to #3723 to make it more consistent and simpler.

  • Instead of creating a fake element type, add "element" to field names directly
  • To support direct control of fieldNames, move name handling into ApplyNameMapping
  • Remove unnecessary method, makeListType
  • Apply similar changes to maps to avoid name mismatch bugs in the future

@rdblue
Copy link
Contributor Author

rdblue commented Dec 19, 2021

@RussellSpitzer can you take a look at this follow-up to #3723?

@rdblue
Copy link
Contributor Author

rdblue commented Dec 19, 2021

@SinghAsDev FYI

@SinghAsDev
Copy link
Contributor

Thanks @rdblue , this makes sense. I like that we don't have to modify/ construct parquet types with this change.

I have a couple more changes along this, and would like to get your reviews on them. Will likely post them today.

@Override
public void beforeElementField(Type element) {
super.beforeElementField(makeElement(element));
// normalize the name to "element" so that the mapping will match structures with alternative names
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdblue would it help if we add some example of this alternate naming source here? Same for the map's key and value fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. The main point is that we don't want to care what the other name was.

@rdblue rdblue merged commit a0b57d5 into apache:master Dec 21, 2021
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