Copilot commented on code in PR #63320:
URL: https://github.com/apache/doris/pull/63320#discussion_r3254027624
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -191,7 +191,7 @@ public List<String> listTableNames(String dbName) {
// IcebergMetadataOps handles listTableNames and listViewNames
separately.
// listTableNames should only focus on the table type,
// but in reality, Iceberg's return includes views. Therefore,
we added a filter to exclude views.
- if (catalog instanceof ViewCatalog) {
+ if (isViewCatalogEnabled()) {
views = ((ViewCatalog)
catalog).listViews(getNamespace(dbName))
.stream().map(TableIdentifier::name).collect(Collectors.toList());
} else {
Review Comment:
`listTableNames` previously filtered views whenever `catalog instanceof
ViewCatalog` because `listTables` may include views (per the comment
immediately above). With the new `isViewCatalogEnabled()` gate, when
`iceberg.rest.view-enabled=false` the code no longer collects view names and
therefore cannot filter them out, which can cause `listTableNames` to return
view names as tables. If the goal is “don’t break when view ops are
disabled/unavailable”, consider still attempting the view filtering when
`catalog instanceof ViewCatalog`, but make it best-effort (e.g., catch/handle
failures from `listViews` and proceed), or adjust `listTables` handling to
reliably exclude views without relying on `listViews`.
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/iceberg/IcebergMetadataOpTest.java:
##########
@@ -45,4 +58,60 @@ public void testGetNamespaces() {
ns = IcebergMetadataOps.getNamespace(Optional.empty(), "");
Assert.assertEquals(0, ns.length());
}
+
+ @Test
+ public void testListTableNamesSkipsViewsWhenRestViewDisabled() {
Review Comment:
The test name claims it “skips views”, but the stubbed `listTables` response
only contains a table (no view), so the test is really asserting that
`listViews` is not called when `iceberg.rest.view-enabled=false`. Either rename
it to reflect the behavior being tested (e.g., “doesNotCallListViews...”), or
include a view identifier in `listTables` and assert the returned list excludes
it (if that’s the intended contract).
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -1193,12 +1193,25 @@ private Namespace getNamespace() {
return externalCatalogName.map(Namespace::of).orElseGet(() ->
Namespace.empty());
}
+ private boolean isViewCatalogEnabled() {
+ if (!(catalog instanceof ViewCatalog)) {
+ return false;
+ }
+ if (dorisCatalog instanceof IcebergRestExternalCatalog) {
+ MetastoreProperties metaProps =
dorisCatalog.getCatalogProperty().getMetastoreProperties();
+ if (metaProps instanceof IcebergRestProperties) {
+ return ((IcebergRestProperties)
metaProps).isIcebergRestViewEnabled();
+ }
+ }
+ return true;
+ }
+
public ThreadPoolExecutor getThreadPoolWithPreAuth() {
return dorisCatalog.getThreadPoolWithPreAuth();
}
private void performDropView(String remoteDbName, String remoteViewName)
throws DdlException {
- if (!(catalog instanceof ViewCatalog)) {
+ if (!isViewCatalogEnabled()) {
throw new DdlException("Drop Iceberg view is not supported with
not view catalog.");
Review Comment:
The exception message is grammatically incorrect and unclear (“with not view
catalog”). Consider rephrasing to something like “Drop Iceberg view is not
supported when the catalog does not support views.” or “... when view
operations are disabled.”
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/property/metastore/IcebergRestPropertiesTest.java:
##########
@@ -83,6 +83,23 @@ public void testVendedCredentialsDisabled() {
Assertions.assertFalse(catalogProps.containsKey("header.X-Iceberg-Access-Delegation"));
}
+ @Test
+ public void testRestViewEnabled() {
+ Map<String, String> props = new HashMap<>();
+ props.put("iceberg.rest.uri", "http://localhost:8080";);
+
+ IcebergRestProperties defaultProps = new IcebergRestProperties(props);
+ defaultProps.initNormalizeAndCheckProps();
+ Assertions.assertTrue(defaultProps.isIcebergRestViewEnabled());
+
+ props.put("iceberg.rest.view-enabled", "false");
+ IcebergRestProperties disabledProps = new IcebergRestProperties(props);
Review Comment:
This test mutates the same `props` map after constructing `defaultProps`. If
`IcebergRestProperties` retains a reference to the input map (rather than
copying), later mutations can make the test brittle and harder to reason about.
Consider using a fresh map for the disabled case (or copying the original map)
to keep each assertion independent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
