Copilot commented on code in PR #63319:
URL: https://github.com/apache/doris/pull/63319#discussion_r3254027015
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -190,7 +190,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:
This changes the filtering behavior: when `catalog` is a `ViewCatalog` but
view operations are disabled, `listTableNames` will no longer fetch
`listViews`, which can let view identifiers returned by `listTables()` pass
through unfiltered. If Iceberg REST can still include views in `listTables()`
even when view ops are disabled, this will incorrectly return views as tables.
A concrete fix is to keep filtering logic consistent (e.g., attempt `listViews`
and handle failures explicitly), or update the comment + behavior to guarantee
that `listTables()` cannot include views in this mode.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -1182,12 +1182,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. Consider
rewording to something actionable like “Dropping Iceberg views is not supported
when view operations are disabled (iceberg.rest.view-enabled=false) or the
catalog does not support views.” so users know how to fix it.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -1182,12 +1182,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;
Review Comment:
`isViewCatalogEnabled()` is called from multiple hot-path methods
(listing/existence checks). If `getCatalogProperty().getMetastoreProperties()`
is non-trivial, consider caching the computed boolean in the constructor or
lazily memoizing it to avoid repeated property resolution.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -1116,7 +1116,7 @@ public Table loadTable(String dbName, String tblName) {
@Override
public boolean viewExists(String remoteDbName, String remoteViewName) {
- if (!(catalog instanceof ViewCatalog)) {
+ if (!isViewCatalogEnabled()) {
return false;
}
Review Comment:
New view-enabled gating affects multiple behaviors beyond `listTableNames`
(e.g., `viewExists`, `loadView`, `listViewNames`, and `performDropView`).
Please add unit tests covering these code paths for
`iceberg.rest.view-enabled=false` to ensure consistent behavior
(false/null/empty/exception) and prevent regressions.
--
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]
