github-actions[bot] commented on code in PR #68510:
URL: https://github.com/apache/doris/pull/68510#discussion_r4100633124
##########
regression-test/suites/external_table_p0/adbc/test_adbc_metadata_ops.groovy:
##########
@@ -194,6 +194,15 @@ suite("test_adbc_metadata_ops", "p0,external") {
sql """DESC ${catalogName}.${sqliteDb}.meta_a"""
sqliteExec("ALTER TABLE meta_a ADD COLUMN added_by_refresh_table TEXT;"
+ " UPDATE meta_a SET added_by_refresh_table = 'x';")
+
+ // The DESC above paid for this schema once. Until a REFRESH arrives
the connector keeps serving
+ // that copy, which is the half that gives the assertion below its
meaning -- a connector that
+ // re-read on every statement would satisfy that one while remembering
nothing. The DATABASE and
+ // CATALOG levels below assert only the positive half: they exercise
the same rule at a coarser key.
+ def columnsBeforeRefresh = sql("DESC
${catalogName}.${sqliteDb}.meta_a").collect { it[0] } as Set
Review Comment:
[P2] This check is masked by fe-core's schema cache, so it does not prove
the ADBC metadata layer uses its own cache. The first `DESC` populates
`ExternalMetaCacheMgr`; the second one is normally answered there without
calling `AdbcConnectorMetadata` at all. Even if `arrowSchemaOf` stopped calling
`cache.tableSchema(...)` and fetched remotely every time, this pre-refresh
assertion would remain stale, and the post-refresh assertion would pass because
REFRESH clears the FE cache. Since this PR deletes the native test that used
fresh metadata instances sharing only `AdbcMetadataCache`, please retain a
pure-Java connector-level test with a recording schema source (or otherwise
bypass the FE schema cache) to cover that integration.
##########
fe/fe-connector/fe-connector-adbc/src/test/java/org/apache/doris/connector/adbc/AdbcConnectorMetadataNativeTest.java:
##########
@@ -0,0 +1,107 @@
+// 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.adbc;
+
+import org.apache.doris.connector.spi.DorisConnectorException;
+import org.apache.doris.thrift.TTableDescriptor;
+import org.apache.doris.thrift.TTableType;
+
+import org.apache.arrow.adbc.core.AdbcStatement;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.nio.file.Path;
+import java.util.Map;
+
+/**
+ * The two metadata cases that need a real driver but never read a result: a
missing table must not be
+ * reported as a driver gap, and the table descriptor handed to the scan path
must be typed.
+ *
+ * <p>Both stop before any Arrow data is materialized -- the first ends in a
thrown exception, the second
+ * never asks the source -- which is what makes them safe to run in FE UT. See
{@link AdbcNativeTestSupport}
+ * for the rule: a test that iterates an {@code ArrowReader} also needs
arrow-c-data's JNI shim, which is an
+ * upstream binary that cannot load on every host Doris supports, and those
tests belong in the regression
+ * suites instead. The rest of the metadata surface -- listings, schema
mapping, views, handles -- is
+ * asserted end to end by {@code
regression-test/suites/external_table_p0/adbc}, which is where it can be.
+ */
+class AdbcConnectorMetadataNativeTest {
+
+ private static AdbcClient sqliteClient(Path dbFile) {
+ return new AdbcClient(AdbcNativeTestSupport.sqliteDriver(),
"libadbc_driver_sqlite.so",
+ null, "file:" + dbFile, null, null, Map.of());
+ }
+
+ /**
+ * A fresh cache per call, so each test reads the source rather than an
earlier test's answers.
+ */
+ private static AdbcConnectorMetadata metadataOn(AdbcClient client) {
+ return new AdbcConnectorMetadata(client, new AdbcSchemaStrategy(),
+ AdbcDialectRegistry::defaultDialect, new
AdbcMetadataCache(Map.of()));
+ }
+
+ private static void seed(AdbcClient client) {
+ client.withConnection(connection -> {
+ for (String sql : new String[] {
+ "CREATE TABLE IF NOT EXISTS t1 (c_int INTEGER, c_dbl REAL,
c_txt TEXT, c_blob BLOB)",
+ "INSERT INTO t1 VALUES (1, 1.5, 'a', x'00ff')",
+ "CREATE TABLE IF NOT EXISTS t2 (a INTEGER)",
+ "CREATE VIEW IF NOT EXISTS v1 AS SELECT * FROM t1"}) {
+ try (AdbcStatement statement = connection.createStatement()) {
+ statement.setSqlQuery(sql);
+ statement.executeUpdate();
+ }
+ }
+ return null;
+ });
+ }
+
+ @Test
+ void missingTableIsReportedAsSuchNotAsADriverGap(@TempDir Path tempDir) {
+ try (AdbcClient client = sqliteClient(tempDir.resolve("meta.db"))) {
+ seed(client);
+ AdbcConnectorMetadata metadata = metadataOn(client);
+ // Build a handle for a table that does not exist, bypassing
getTableHandle's existence check.
+ AdbcTableHandle ghost = new AdbcTableHandle(new
AdbcNamespace("main", ""), "no_such_table");
+
+ DorisConnectorException e =
Assertions.assertThrows(DorisConnectorException.class,
+ () -> metadata.getTableSchema(null, ghost));
+
+ // The fallback to executeSchema must fire only on
NOT_IMPLEMENTED. Falling back on every error
+ // would answer a plain missing table with "this driver implements
neither method", sending the
+ // user to look at their driver instead of their table name.
+ Assertions.assertTrue(e.getMessage().contains("no_such_table"),
e.getMessage());
+ Assertions.assertFalse(e.getMessage().contains("implements
neither"), e.getMessage());
+ }
+ }
+
+ @Test
+ void tableDescriptorIsTypedForTheScanPath(@TempDir Path tempDir) {
+ try (AdbcClient client = sqliteClient(tempDir.resolve("meta.db"))) {
Review Comment:
[P2] Keep this descriptor assertion independent of the native libraries.
`buildTableDescriptor` only constructs the Thrift descriptor from its arguments
and never touches `client`, but entering this block calls `sqliteClient()` (and
then `seed()`); `sqliteClient()` reaches `requireNativeLibraryDir()`, so an FE
UT host without the optional ADBC `.so` files skips this otherwise pure
contract test. Please construct metadata with a non-opening client (or move
this case to a pure metadata test) and omit the seed so the
`HIVE_TABLE`/`hiveTable` guarantee runs everywhere.
##########
regression-test/suites/external_table_p0/adbc/test_adbc_catalog_scan.groovy:
##########
@@ -80,10 +80,22 @@ suite("test_adbc_catalog_scan", "p0,external") {
String catalogName = "test_adbc_catalog_scan_catalog"
String dbName = "test_adbc_catalog_scan_db"
+ // A second database in the SOURCE, so the listing has a namespace it must
not mix into the other one.
+ // Created up front, so nothing below depends on a database appearing
behind Doris's back.
Review Comment:
[P2] Please preserve a check that `listDatabaseNames` reads the source
rather than the connector cache. Both source databases are created before this
catalog and its first database lookup, and no surviving test mutates the source
namespaces after that cache is populated. Consequently, changing
`listDatabaseNames` from `reloadNamespaces(...)` to `namespaces(...)` leaves
these additions green, while the deleted native test explicitly caught a
stale/ghost cached namespace. A direct connector-level repeated listing with a
recording source, or an FE lookup/query of a newly created remote database that
forces the name-cache miss reload, would retain that contract.
##########
fe/fe-connector/fe-connector-adbc/src/test/java/org/apache/doris/connector/adbc/AdbcMetadataCacheNativeTest.java:
##########
@@ -1,184 +0,0 @@
-// 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.adbc;
-
-import org.apache.doris.connector.spi.ConnectorColumn;
-import org.apache.doris.connector.spi.ConnectorTableSchema;
-import org.apache.doris.connector.spi.handle.ConnectorTableHandle;
-
-import org.apache.arrow.adbc.core.AdbcStatement;
-import org.junit.jupiter.api.Assertions;
-import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.io.TempDir;
-
-import java.nio.file.Path;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-
-/**
- * The metadata path with a catalog-level cache in front of it, against the
real SQLite driver.
- *
- * <p>Each test changes the source behind Doris's back and then asks what
Doris sees. That is the only
- * evidence that says whether an answer came from memory or from the driver,
and unlike a call counter it
- * cannot be satisfied by a cache that stores things it never reads.
- *
- * <p>Every {@code metadata()} call stands for one statement: the engine
builds a fresh
- * {@link AdbcConnectorMetadata} per statement, and the cache is what they
share.
- *
- * <p>Skips loudly when thirdparty's native libraries are absent -- see {@link
AdbcNativeTestSupport}.
- */
-class AdbcMetadataCacheNativeTest {
-
- private final AdbcMetadataCache cache = new AdbcMetadataCache(Map.of());
-
- private static AdbcClient sqliteClient(Path dbFile) {
- return new AdbcClient(AdbcNativeTestSupport.sqliteDriver(),
"libadbc_driver_sqlite.so",
- null, "file:" + dbFile, null, null, Map.of());
- }
-
- /** One statement's view of the catalog. Separate objects, one shared
cache -- as in production. */
- private AdbcConnectorMetadata metadata(AdbcClient client) {
- return new AdbcConnectorMetadata(client, new AdbcSchemaStrategy(),
- AdbcDialectRegistry::defaultDialect, cache);
- }
-
- private static void execute(AdbcClient client, String... statements) {
- client.withConnection(connection -> {
- for (String sql : statements) {
- try (AdbcStatement statement = connection.createStatement()) {
- statement.setSqlQuery(sql);
- statement.executeUpdate();
- }
- }
- return null;
- });
- }
-
- /** SQLite derives its Arrow types from the values present, so a row is
needed for the types to be real. */
- private static void seed(AdbcClient client) {
- execute(client,
- "CREATE TABLE t1 (c_int INTEGER, c_txt TEXT)",
- "INSERT INTO t1 VALUES (1, 'a')");
- }
-
- private static List<String> columnNames(ConnectorTableSchema schema) {
- List<String> names = new ArrayList<>();
- for (ConnectorColumn column : schema.getColumns()) {
- names.add(column.getName());
- }
- return names;
- }
-
- private List<String> columnsOf(AdbcClient client, String table) {
- ConnectorTableHandle handle = metadata(client).getTableHandle(null,
"main", table).orElseThrow();
- return columnNames(metadata(client).getTableSchema(null, handle));
- }
-
- @Test
- void theNextStatementReadsTheSchemaTheLastOneAlreadyPaidFor(@TempDir Path
tempDir) {
- try (AdbcClient client = sqliteClient(tempDir.resolve("cache.db"))) {
- seed(client);
- Assertions.assertEquals(List.of("c_int", "c_txt"),
columnsOf(client, "t1"));
-
- execute(client, "ALTER TABLE t1 ADD COLUMN c_added INTEGER");
-
- // The column really is there now -- the source changed and Doris
was not told. Serving the
- // remembered shape is the whole point; noticing the change here
would mean nothing was cached.
- Assertions.assertEquals(List.of("c_int", "c_txt"),
columnsOf(client, "t1"));
- }
- }
-
- @Test
- void refreshTableIsWhatMakesTheAlteredColumnsVisible(@TempDir Path
tempDir) {
- try (AdbcClient client = sqliteClient(tempDir.resolve("cache.db"))) {
- seed(client);
- columnsOf(client, "t1");
- execute(client, "ALTER TABLE t1 ADD COLUMN c_added INTEGER");
-
- cache.invalidateTable("main", "t1");
-
- Assertions.assertEquals(List.of("c_int", "c_txt", "c_added"),
columnsOf(client, "t1"));
- }
- }
-
- /**
- * Decision C. Reading the listing from memory is fine; concluding from
memory that a name does not exist
- * is not. A user who just created a table and is told it is not there has
no way to tell that from a
- * typo, and no reason to suspect a cache.
- */
- @Test
- void tableCreatedAfterTheListingWasCachedIsStillFound(@TempDir Path
tempDir) {
- try (AdbcClient client = sqliteClient(tempDir.resolve("cache.db"))) {
- seed(client);
- metadata(client).listTableNames(null, "main");
-
- execute(client, "CREATE TABLE t_new (a INTEGER)", "INSERT INTO
t_new VALUES (1)");
-
- Optional<ConnectorTableHandle> handle =
metadata(client).getTableHandle(null, "main", "t_new");
Review Comment:
[P2] Please retain a portable test for `getTableHandle`'s own last-chance
reload before deleting this case. The created-later regression queries do not
reach that branch: fe-core first refreshes `ExternalDatabase`'s missing name
through `AdbcConnectorMetadata.listTableNames`, which calls
`cache.reloadTableNames`; by the time `getTableHandle` runs, its initial
`cache.tableNames` lookup already contains the new table. Removing the fallback
reload in `tableExists` would therefore leave those regressions green. A
pure-Java recording source can seed a stale connector listing, expose a new
table, call `getTableHandle` directly, and assert the source is reread.
--
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]
