Skip to content

Replace PartitionValues class with record #14796

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.

Sign up for GitHub

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

Closed
findinpath wants to merge 2 commits into from
Closed

Replace PartitionValues class with record #14796

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5225dd
Replace PartitionValues class with record
findinpath Oct 27, 2022
fc37068
Replace PartitionValues class with record
findinpath Oct 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 27 additions & 46 deletions ...n/trino-hive/src/test/java/io/trino/plugin/hive/metastore/glue/TestHiveGlueMetastore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@

import java.io.File;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -109,8 +108,8 @@
import static io.trino.spi.type.VarcharType.createUnboundedVarcharType;
import static java.lang.String.format;
import static java.lang.System.currentTimeMillis;
import static java.util.Arrays.asList;
import static java.util.Locale.ENGLISH;
import static java.util.Objects.requireNonNull;
import static java.util.UUID.randomUUID;
import static java.util.concurrent.TimeUnit.DAYS;
import static org.apache.hadoop.hive.common.FileUtils.makePartName;
Expand Down Expand Up @@ -724,21 +723,21 @@ public void testGetPartitionsFilterTwoPartitionKeys()
CREATE_TABLE_COLUMNS_PARTITIONED_TWO_KEYS,
ImmutableList.of(PARTITION_KEY, PARTITION_KEY2),
ImmutableList.of(
PartitionValues.make("2020-01-01", "100"),
PartitionValues.make("2020-02-01", "200"),
PartitionValues.make("2020-03-01", "300"),
PartitionValues.make("2020-04-01", "400")),
new PartitionValues("2020-01-01", "100"),
new PartitionValues("2020-02-01", "200"),
new PartitionValues("2020-03-01", "300"),
new PartitionValues("2020-04-01", "400")),
ImmutableList.of(equalsFilter, rangeFilter, TupleDomain.all()),
ImmutableList.of(
ImmutableList.of(PartitionValues.make("2020-03-01", "300")),
ImmutableList.of(new PartitionValues("2020-03-01", "300")),
ImmutableList.of(
PartitionValues.make("2020-03-01", "300"),
PartitionValues.make("2020-04-01", "400")),
new PartitionValues("2020-03-01", "300"),
new PartitionValues("2020-04-01", "400")),
ImmutableList.of(
PartitionValues.make("2020-01-01", "100"),
PartitionValues.make("2020-02-01", "200"),
PartitionValues.make("2020-03-01", "300"),
PartitionValues.make("2020-04-01", "400"))));
new PartitionValues("2020-01-01", "100"),
new PartitionValues("2020-02-01", "200"),
new PartitionValues("2020-03-01", "300"),
new PartitionValues("2020-04-01", "400"))));
}

@Test
Expand Down Expand Up @@ -773,12 +772,12 @@ public void testGetPartitionsFilterTwoPartitionKeysPartialQuery()
CREATE_TABLE_COLUMNS_PARTITIONED_TWO_KEYS,
ImmutableList.of(PARTITION_KEY, PARTITION_KEY2),
ImmutableList.of(
PartitionValues.make("2020-01-01", "100"),
PartitionValues.make("2020-02-01", "200"),
PartitionValues.make("2020-03-01", "300"),
PartitionValues.make("2020-04-01", "400")),
new PartitionValues("2020-01-01", "100"),
new PartitionValues("2020-02-01", "200"),
new PartitionValues("2020-03-01", "300"),
new PartitionValues("2020-04-01", "400")),
ImmutableList.of(equalsFilter),
ImmutableList.of(ImmutableList.of(PartitionValues.make("2020-03-01", "300"))));
ImmutableList.of(ImmutableList.of(new PartitionValues("2020-03-01", "300"))));
}

@Test
Expand Down Expand Up @@ -968,12 +967,12 @@ public void testGetPartitionsFilterEqualsAndIsNotNull()
CREATE_TABLE_COLUMNS_PARTITIONED_TWO_KEYS,
ImmutableList.of(PARTITION_KEY, PARTITION_KEY2),
ImmutableList.of(
PartitionValues.make("2020-01-01", "100"),
PartitionValues.make("2020-02-01", "200"),
PartitionValues.make("2020-03-01", "300"),
PartitionValues.make(null, "300")),
new PartitionValues("2020-01-01", "100"),
new PartitionValues("2020-02-01", "200"),
new PartitionValues("2020-03-01", "300"),
new PartitionValues(null, "300")),
ImmutableList.of(equalsAndIsNotNullFilter),
ImmutableList.of(ImmutableList.of(PartitionValues.make("2020-03-01", "300"))));
ImmutableList.of(ImmutableList.of(new PartitionValues("2020-03-01", "300"))));
}

@Test
Expand Down Expand Up @@ -1347,11 +1346,11 @@ private void doGetPartitionsFilterTest(
throws Exception
{
List<PartitionValues> partitionValuesList = partitionStringValues.stream()
.map(PartitionValues::make)
.map(PartitionValues::new)
.collect(toImmutableList());
List<List<PartitionValues>> expectedPartitionValuesList = expectedSingleValueList.stream()
.map(expectedValue -> expectedValue.stream()
.map(PartitionValues::make)
.map(PartitionValues::new)
.collect(toImmutableList()))
.collect(toImmutableList());
doGetPartitionsFilterTest(columnMetadata, ImmutableList.of(partitionColumnName), partitionValuesList, filterList, expectedPartitionValuesList);
Expand Down Expand Up @@ -1379,7 +1378,7 @@ private void doGetPartitionsFilterTest(
TupleDomain<String> filter = filterList.get(i);
List<PartitionValues> expectedValues = expectedValuesList.get(i);
List<String> expectedResults = expectedValues.stream()
.map(expectedPartitionValues -> makePartName(partitionColumnNames, expectedPartitionValues.getValues()))
.map(expectedPartitionValues -> makePartName(partitionColumnNames, asList(expectedPartitionValues.values)))
.collect(toImmutableList());

Optional<List<String>> partitionNames = metastoreClient.getPartitionNamesByFilter(
Expand Down Expand Up @@ -1407,7 +1406,7 @@ private void createDummyPartitionedTable(SchemaTableName tableName, List<ColumnM
List<PartitionWithStatistics> partitions = new ArrayList<>();
List<String> partitionNames = new ArrayList<>();
partitionValues.stream()
.map(partitionValue -> makePartName(partitionColumnNames, partitionValue.values))
.map(partitionValue -> makePartName(partitionColumnNames, asList(partitionValue.values)))
.forEach(
partitionName -> {
partitions.add(new PartitionWithStatistics(createDummyPartition(table, partitionName), partitionName, PartitionStatistics.empty()));
Expand Down Expand Up @@ -1441,24 +1440,6 @@ public void close()
}
}

// container class for readability. Each value is one for a partitionKey, in order they appear in the schema
private static class PartitionValues
{
private final List<String> values;

private static PartitionValues make(String... values)
{
return new PartitionValues(Arrays.asList(values));
}

private PartitionValues(List<String> values)
{
this.values = ImmutableList.copyOf(requireNonNull(values, "values is null"));
}

public List<String> getValues()
{
return values;
}
private record PartitionValues(String... values){
Copy link
Copy Markdown
Member

@findepi findepi Oct 27, 2022

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
Copy Markdown
Contributor Author

@findinpath findinpath Oct 28, 2022

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
Copy Markdown
Member

@losipiuk losipiuk Oct 28, 2022

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
Copy Markdown
Contributor Author

@findinpath findinpath Oct 28, 2022

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.

}
}