Core: Update ViewMetadata builder and tests#135
Conversation
| @Value.Derived | ||
| default Map<Integer, ViewVersion> versionsById() { | ||
| return Builder.indexVersions(versions()); | ||
| ImmutableMap.Builder<Integer, ViewVersion> builder = ImmutableMap.builder(); |
There was a problem hiding this comment.
No need for separate static methods now that these are used directly by the builder in its constructor.
| private final List<Schema> schemas; | ||
| private final List<ViewHistoryEntry> history; | ||
| private final Map<String, String> properties; | ||
| private final List<MetadataUpdate> changes; |
There was a problem hiding this comment.
Some reformatting here to keep final variables separate.
| this.versions = Lists.newArrayList(base.versions()); | ||
| this.versionsById = Maps.newHashMap(base.versionsById()); | ||
| this.schemas = Lists.newArrayList(base.schemas()); | ||
| this.schemasById = Maps.newHashMap(base.schemasById()); |
There was a problem hiding this comment.
Rather than running the index method each time a schema or version is added, this pulls the index from the last version and keeps it up to date.
|
|
||
| this.formatVersion = newFormatVersion; | ||
| this.changes.add(new MetadataUpdate.UpgradeFormatVersion(newFormatVersion)); | ||
| changes.add(new MetadataUpdate.UpgradeFormatVersion(newFormatVersion)); |
There was a problem hiding this comment.
A few style changes to remove this.
| private List<Schema> schemas = Lists.newArrayList(); | ||
| private int currentVersionId; | ||
| private Integer lastAddedVersionId; | ||
| private Integer lastAddedSchemaId; |
There was a problem hiding this comment.
Tracking the last added schema ID was not necessary because we never set the schema ID directly. Now the schema is added at the same time a new version is added in setCurrentVersion(ViewVersion, Schema).
| // changes can be added | ||
| int highestFieldId = highestFieldId(); | ||
| for (Schema schema : schemas) { | ||
| this.changes.add(new MetadataUpdate.AddSchema(schema, highestFieldId)); |
There was a problem hiding this comment.
The AddSchema change only needs the highest field ID with respect to the existing schemas and the new schema, so these changes can be added in addSchema.
| } | ||
|
|
||
| return newVersionId; | ||
| public Builder addVersion(ViewVersion version) { |
There was a problem hiding this comment.
Moved some methods around to keep version and schema methods located near one another.
| .anyMatch(added -> added.viewVersion().versionId() == newVersionId); | ||
| this.lastAddedVersionId = isNewVersion ? newVersionId : null; | ||
| if (versionsById.containsKey(newVersionId)) { | ||
| boolean addedInBuilder = |
There was a problem hiding this comment.
The check for lastAddedVersionId != null was redundant, so I removed it. Now it's easier to read.
| return newVersionId; | ||
| } | ||
|
|
||
| Preconditions.checkArgument( |
There was a problem hiding this comment.
I moved these checks to more appropriate locations.
|
|
||
| // expire old versions, but keep at least the versions added in this builder | ||
| int numAddedVersions = (int) changes(MetadataUpdate.AddViewVersion.class).count(); | ||
| int numVersionsToKeep = Math.max(numAddedVersions, historySize); |
There was a problem hiding this comment.
This is a behavior change. If more versions were added than should be retained, at least one metadata file should contain them.
| retainedVersions = expireVersions(versionsById, numVersionsToKeep); | ||
| Set<Integer> retainedVersionIds = | ||
| retainedVersions.stream().map(ViewVersion::versionId).collect(Collectors.toSet()); | ||
| retainedHistory = updateHistory(history, retainedVersionIds); |
There was a problem hiding this comment.
I refactored these. History now uses logic similar to tables. And versions are expired by ID since IDs are assigned in an increasing order. That way we always keep the most recent versions rather than the ones that happened to be at the start of the list.
| formatVersion, | ||
| location, | ||
| schemas, | ||
| ImmutableList.copyOf(schemas), |
There was a problem hiding this comment.
no need to do this, as those will be immutable collections internally, which is being handled when the ViewMetadata instance is created
2 participants
This implements the remaining changes need to get apache#8147 in.