Implementation of version metadata table for view #12014
Conversation
|
Hi, @amogh-jahagirdar @nastra Could I get some feedback for this PR? Thanks |
|
@huan233usc sorry for the delay here. I'll try to review this either today or early next week |
|
@huan233usc could you resolve the merge conflicts please? |
|
merge conflicts resolved |
|
|
||
| @Override | ||
| public FileIO io() { | ||
| return null; |
There was a problem hiding this comment.
pretty much all methods that aren't supported by views should be throwing a UnsupportedOperationException (similar to how it's done in the super class
There was a problem hiding this comment.
Agree, we should remove all of the concrete implementations that are returning null here.
|
|
||
| TableScan scan = viewVersionTable.newScan(); | ||
|
|
||
| assertThat(scan.schema()).isEqualTo(ViewVersionTable.VIEW_VERSION_SCHEMA); |
There was a problem hiding this comment.
this should have some additional checks, such as
List<ViewMetadataReadTask> scanTasks =
Lists.newArrayList(
Iterators.transform(
scan.planFiles().iterator(),
task -> {
assertThat(task).isInstanceOf(ViewMetadataReadTask.class);
return (ViewMetadataReadTask) task;
}));
assertThat(scanTasks).hasSize(1);
ViewVersion viewVersion = viewVersionTable.operations().current().currentVersion();
assertThat(scanTasks.get(0).rows())
.containsExactly(
ViewMetadataReadTask.Row.of(
viewVersion.versionId(),
viewVersion.schemaId(),
viewVersion.timestampMillis() * 1000,
viewVersion.summary(),
viewVersion.defaultCatalog(),
viewVersion.representations(),
viewVersion.defaultNamespace().toString()));
Also it would be good to test scans against the version table where only one/two columns are projected
| public void commit(ViewMetadata base, ViewMetadata updated) {} | ||
| } | ||
|
|
||
| private TestViews() {} |
| } | ||
|
|
||
| @Override | ||
| public void commit(ViewMetadata base, ViewMetadata updated) {} |
There was a problem hiding this comment.
we should probably also throw a UOE if commit isn't supported
| .versionId(version) | ||
| .schemaId(0) | ||
| .timestampMillis(timestampMillis) | ||
| .defaultNamespace(Namespace.of("foo.bar")) |
There was a problem hiding this comment.
| .defaultNamespace(Namespace.of("foo.bar")) | |
| .defaultNamespace(Namespace.of("foo", "bar")) |
|
|
||
| public static class TestViewOperations implements ViewOperations { | ||
|
|
||
| private ViewMetadata metadata; |
There was a problem hiding this comment.
| private ViewMetadata metadata; | |
| private final ViewMetadata metadata; |
|
|
||
| public static TestView createSampleTestView(String name) { | ||
| return new TestView( | ||
| new TestViewOperations(/* version= */ 1, /* timestampMillis= */ 1737436503101L), name); |
There was a problem hiding this comment.
the inlined comment typically goes after the actual value, but it should be ok omitting the comment here
|
|
||
| assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue(); | ||
| Table table = tableCatalog().loadTable(TableIdentifier.of("ns", "view", "version")); | ||
| assertThat(table).isInstanceOf(ViewVersionTable.class); |
There was a problem hiding this comment.
this here should probably also have some additional checks
| viewCatalog | ||
| .buildView(TableIdentifier.of(NAMESPACE, viewName)) | ||
| .withQuery("spark", sql) | ||
| // use non-existing column name to make sure only the SQL definition for spark is loaded |
There was a problem hiding this comment.
this comment is probably not really relevant for this test here
| // Similar to table's metadata table, view's metadata table requires fully qualified name. | ||
| List<Object[]> result = sql("SELECT * FROM %s.%s.%s.version", catalogName, NAMESPACE, viewName); | ||
| assertThat(result).hasSize(1); | ||
| assertThat(result.get(0).length).isEqualTo(7); |
There was a problem hiding this comment.
we should add some additional checks here to assert all the values are what we expect. For this, you can store the result of the view creation above in a view variable and then use view.currentVersion() to compare values
| @@ -2109,6 +2109,34 @@ public void createViewWithCustomMetadataLocation() throws IOException { | |||
| "")); | |||
| } | |||
|
|
|||
| @Test | |||
| public void readFromViewVersionTable() throws NoSuchTableException { | |||
There was a problem hiding this comment.
we should also have a test where only one/two columns are projected
| View baseView = loadView(context, baseViewIdentifier); | ||
| return MetadataTableUtils.createViewMetadataTableInstance(baseView, tableName, type); | ||
| } else { | ||
| throw new NoSuchTableException("Table does not exist: %s", identifier); |
There was a problem hiding this comment.
we should probably throw the original exception that was caught in loadTable
| return MetadataTableUtils.createViewMetadataTableInstance( | ||
| ops, name(), baseViewIdentifier, identifier, type); | ||
| } else { | ||
| throw new NoSuchTableException("Table does not exist: %s", identifier); |
There was a problem hiding this comment.
we should probably also throw the original exception that was caught in loadTable
| ViewOperations ops = newViewOps(baseViewIdentifier); | ||
| if (ops.current() == null) { | ||
| throw new NoSuchTableException("Table or View does not exist: %s", baseViewIdentifier); | ||
| } |
There was a problem hiding this comment.
nit: please add a new line after the }
|
|
||
| @Override | ||
| public CloseableIterable<FileScanTask> planFiles() { | ||
| // override planFiles to avoid the check for a current snapshot because this metadata table is |
There was a problem hiding this comment.
I don't think this comment applies for views, so it can be removed
| this.rows = rows; | ||
| } | ||
|
|
||
| ViewMetadataReadTask( |
| return ImmutableList.of(this); | ||
| } | ||
|
|
||
| Schema projectedSchema() { |
There was a problem hiding this comment.
this and metadataFile aren't used, so can be removed
| @@ -462,6 +463,26 @@ private LoadTableResponse loadInternal( | |||
|
|
|||
| @Override | |||
| public Table loadTable(SessionContext context, TableIdentifier identifier) { | |||
| try { | |||
| return loadTableInternal(context, identifier); | |||
| } catch (NoSuchTableException original) { | |||
There was a problem hiding this comment.
I don't think is the right way to fallback to loading the metadata table. If you look at the current fallback logic metadata tables, we should attempt loading in that same path.
| ViewMetadataTableType type = ViewMetadataTableType.from(tableName); | ||
| if (type != null) { | ||
| TableIdentifier baseViewIdentifier = TableIdentifier.of(identifier.namespace().levels()); | ||
| View baseView = loadView(context, baseViewIdentifier); |
There was a problem hiding this comment.
I believe this will break for catalogs not implementing views, because it's going to throw.
| try { | ||
| return super.loadTable(identifier); | ||
| } catch (NoSuchTableException e) { | ||
| return loadViewMetadataTable(identifier); |
There was a problem hiding this comment.
Similar to the other comment. I don't feel this is the right way to handle metadata tables for views. It should be handled as part of the metadata table fallback logic.
|
|
||
| @Override | ||
| public SortOrder sortOrder() { | ||
| return sortOrder; |
There was a problem hiding this comment.
There's no sort order for view, this shouldn't be included.
|
|
||
| @Override | ||
| public Map<String, String> properties() { | ||
| return ImmutableMap.of(); |
There was a problem hiding this comment.
Views have properties, we should be exposing them.
| @Override | ||
| public Snapshot currentSnapshot() { | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public Iterable<Snapshot> snapshots() { | ||
| return ImmutableList.of(); | ||
| } | ||
|
|
||
| @Override | ||
| public Snapshot snapshot(long snapshotId) { | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public List<HistoryEntry> history() { | ||
| return ImmutableList.of(); | ||
| } | ||
|
|
||
| @Override | ||
| public List<StatisticsFile> statisticsFiles() { | ||
| return ImmutableList.of(); | ||
| } | ||
|
|
||
| @Override | ||
| public List<PartitionStatisticsFile> partitionStatisticsFiles() { | ||
| return ImmutableList.of(); | ||
| } | ||
|
|
||
| @Override | ||
| public Map<String, SnapshotRef> refs() { | ||
| return ImmutableMap.of(); | ||
| } |
There was a problem hiding this comment.
All of these are not handled properly (similar to above)
|
I feel like structurally we're not approaching this the right way. We already have a |
|
Thank you both for the review, will update this pr accordingly |
This PR implements the .version metadata mentioned in #9844
Added integration tests and tested locally using rest catalog
