Core: Add metadata updates for Views / Use own Builder for ViewMetadata #8147
Conversation
nastra
force-pushed
the
view-requirements-and-updates
branch
from
b53ed12 to
f088e47
Compare
nastra
changed the title
nastra
force-pushed
the
view-requirements-and-updates
branch
from
f088e47 to
b57c827
Compare
| private static final String LOCATION = "location"; | ||
|
|
||
| // AddViewVersion | ||
| private static final String VIEW_VERSION = "view-version"; |
There was a problem hiding this comment.
Can you update the REST spec as well?
There was a problem hiding this comment.
I was planning on updating the REST spec as part of #7913 because it requires a bunch of other View-specific stuff and it seems easier to update everything in one go
| return this; | ||
| } | ||
|
|
||
| public Builder schemas(Iterable<Schema> newSchemas) { |
There was a problem hiding this comment.
What does this do differently than addAllSchemas? Do we need a method that adds multiple schemas?
There was a problem hiding this comment.
the difference between setSchemas() and addAllSchemas() is that setSchemas() will replace whatever this.schemas was set to with newSchemas, whereas addAllSchemas() will just add items to this.schemas.
I think we're ok for now to only keep setXyz() and addXyz() without addAllXyz
nastra
force-pushed
the
view-requirements-and-updates
branch
2 times, most recently
from
8490c02 to
b7dc238
Compare
nastra
force-pushed
the
view-requirements-and-updates
branch
from
b7dc238 to
fb378f5
Compare
|
|
||
| public Builder setCurrentVersion(ViewVersion version) { | ||
| Schema schema = indexSchemas(schemas).get(version.schemaId()); | ||
| int newSchemaId = addSchemaInternal(schema); |
There was a problem hiding this comment.
This doesn't seem correct to me.
The ViewVersion references a schema by ID. This method assumes that schema has been added to the builder already (or was already added to the view in a previous commit) because it expects the schema to be present in the ID to schema map produced by indexSchemas. But this then adds the schema and possibly reassigns the schema ID, which will be a noop because the schema is already known.
I think the solution is to pass the schema into this method. Then this would add the schema via addSchemaInternal (possibly reassigning its ID), add the view version that references the schema ID returned by addSchemaInternal, and then call setCurrentVersionId with the view version ID returned by addVersionInternal.
|
@nastra, to avoid more review rounds, I went ahead and fixed the remaining issues in this PR as I was doing a deep dive. See nastra#135 with the changes. You may need to fix a test or two, but I got the |
nastra
force-pushed
the
view-requirements-and-updates
branch
from
9018318 to
a5a8ad0
Compare
Thanks @rdblue, I've updated a few small things as well and pushed them. |
|
Merged. Thanks, @nastra! |
…ta (apache#8147) * Core: Add metadata updates for Views / use own Builder for ViewMetadata * Move validations into Builder to be more lenient when ViewMetadata is read through Parser There are currently only two ways to create an instance of `ViewMetadata`: * through the Builder * through the Parser Almost all validations should be happening in the Builder, while we would like to be more lenient when view metadata is being read (e.g. from disk) via the parser. Reading view metadata through the parser, it is acceptable that things like schemas/versions/history might be empty or that the current version id points to a version that doesn't exist. In such a case we'd rather want to lazily fail when accessing the view's current version rather than when the view metadata is being read by the parser. Co-authored-by: Ryan Blue <blue@apache.org> (cherry picked from commit 6bb5a1a)
2 participants
This PR extracts the
MetadataUpdate/UpdateRequirementchanges from #7913