github-actions[bot] commented on code in PR #63583: URL: https://github.com/apache/doris/pull/63583#discussion_r3296295393
########## fe/fe-core/src/main/java/org/apache/doris/connector/ExternalMetaCacheInvalidator.java: ########## @@ -0,0 +1,82 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connector; + +import org.apache.doris.catalog.Env; +import org.apache.doris.connector.spi.ConnectorMetaInvalidator; +import org.apache.doris.datasource.ExternalMetaCacheMgr; + +import java.util.List; +import java.util.Objects; + +/** + * fe-core side bridge from the connector SPI {@link ConnectorMetaInvalidator} to the + * engine's {@link ExternalMetaCacheMgr}. Returned by + * {@link DefaultConnectorContext#getMetaInvalidator()} so connectors that receive + * external change notifications (e.g. HMS notification events) can drop the right + * cache entries without depending on fe-core internals directly. + */ +public final class ExternalMetaCacheInvalidator implements ConnectorMetaInvalidator { + + private final long catalogId; + + public ExternalMetaCacheInvalidator(long catalogId) { + this.catalogId = catalogId; + } + + @Override + public void invalidateAll() { + mgr().invalidateCatalog(catalogId); + } + + @Override + public void invalidateDatabase(String dbName) { + mgr().invalidateDb(catalogId, Objects.requireNonNull(dbName, "dbName")); + } + + @Override + public void invalidateTable(String dbName, String tableName) { + mgr().invalidateTable(catalogId, + Objects.requireNonNull(dbName, "dbName"), + Objects.requireNonNull(tableName, "tableName")); + } + + @Override + public void invalidatePartition(String dbName, String tableName, List<String> partitionValues) { + // The SPI carries partition column VALUES (e.g. ["2024", "01"]) but the engine's + // partition cache is keyed by partition NAMES (e.g. "year=2024/month=01"). + // Reconstructing the name requires partition column names which are not carried by + // the SPI today. Until the SPI grows that metadata, fall back to table-level + // invalidation — correct but over-broad. + mgr().invalidateTable(catalogId, + Objects.requireNonNull(dbName, "dbName"), + Objects.requireNonNull(tableName, "tableName")); + } + + @Override + public void invalidateStatistics(String dbName, String tableName) { + // ExternalMetaCacheMgr exposes no per-table statistics-only invalidation today Review Comment: The SPI contract says this invalidates cached statistics for the table, but the engine bridge silently does nothing. A connector receiving an external stats/row-count change can call this successfully and Doris will keep serving the old `ExternalRowCountCache` value until its normal refresh/expiry, which defeats the new invalidation API. If stats-only invalidation cannot be implemented yet, this should not be exposed as a successful operation; otherwise resolve the table ids and invalidate the row-count/stat caches, or deliberately fall back to a broader invalidation that is correct. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/PluginDrivenExternalCatalog.java: ########## @@ -232,6 +238,44 @@ public Connector getConnector() { return connector; } + /** + * Routes {@code CREATE TABLE} through the SPI's + * {@code ConnectorTableOps.createTable(session, request)} instead of the + * legacy {@code metadataOps} path used by other {@link ExternalCatalog} + * subclasses. + * + * <p>Connectors that have not overridden the new SPI default fall through + * to the SPI's "CREATE TABLE not supported" exception, which is wrapped + * here as a {@link DdlException} to match the existing caller contract.</p> + * + * <p>The SPI signature is {@code void}: it does not distinguish + * "newly created" from "already existed (IF NOT EXISTS)". This override + * conservatively assumes creation happened and writes the edit log, matching + * the more common branch of the legacy path. Refining this when a connector + * actually needs the distinction is left to P5/P6/P7 connector migrations.</p> + */ + @Override + public boolean createTable(CreateTableInfo createTableInfo) throws UserException { + makeSureInitialized(); + ConnectorSession session = buildConnectorSession(); + ConnectorCreateTableRequest request = CreateTableInfoToConnectorRequestConverter + .convert(createTableInfo, createTableInfo.getDbName()); + try { + connector.getMetadata(session).createTable(session, request); + } catch (DorisConnectorException e) { + throw new DdlException(e.getMessage(), e); + } + org.apache.doris.persist.CreateTableInfo persistInfo = + new org.apache.doris.persist.CreateTableInfo( + getName(), + createTableInfo.getDbName(), + createTableInfo.getTableName()); + Env.getCurrentEnv().getEditLog().logCreateTable(persistInfo); + LOG.info("finished to create table {}.{}.{}", getName(), + createTableInfo.getDbName(), createTableInfo.getTableName()); + return false; + } Review Comment: This always reports `false` ("new table created"), but callers rely on the boolean to skip follow-up work when `CREATE TABLE IF NOT EXISTS` found an existing table. In `CreateTableCommand.run`, the CTAS path returns early only when `Env.createTable(...)` returns `true`; with this override, a plugin connector that no-ops because the table already exists will still make CTAS run the insert into that existing table. That changes `CREATE TABLE IF NOT EXISTS t AS SELECT ...` from "do nothing if t exists" to "insert into t". Please preserve the legacy `ExternalCatalog.createTable` semantics here, e.g. by checking existence around the SPI call or changing the SPI result to carry the already-existed/newly-created state before logging and returning. -- 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]
