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

Replace PartitionValues class with record #14796

Closed

Conversation

findinpath
Copy link
Contributor

@findinpath findinpath commented Oct 27, 2022

Description

Fixes NPE issues caused by the changes introduced in #14742 for copying the partition values list (copying a list containing null values is not supported via guava).

NOTE that TestHiveGlueMetastore needs to be executed with secrets because it tests against AWS Glue.

Non-technical explanation

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Oct 27, 2022
@findinpath findinpath changed the title Replace PartitionValues class with record Replace PartitionValues class with record Oct 27, 2022
@findinpath findinpath self-assigned this Oct 27, 2022
@findepi
Copy link
Member

findepi commented Oct 27, 2022

Fixes NPE issues caused by the changes introduced in #14742 for copying the partition values list (copying a list containing null values is not supported via guava).

I posted a simpler fix for this #14799.

I am OK changing the PartitionValues class into a record, but let's have the two things as separate commits.

{
return values;
}
private record PartitionValues(String... values){
Copy link
Member

Choose a reason for hiding this comment

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

Is PartitionValues no longer immutable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. This change would revert the added immutability in the previous commit. I'll wait for the NPE fix from #14799 to land and use the same logic in the record constructor to make the values actually immutable.

Copy link
Member

Choose a reason for hiding this comment

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

I merged that one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The arrays can be made immutable by converting them to a list.
Changing the code to work with a constructor for PartitionValues based on list defies the purpose of having a concise way to define a variable number of parameters.
It seems to me that the actual design is much better then the one based on record.
Closing the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants