github-actions[bot] commented on code in PR #66628:
URL: https://github.com/apache/doris/pull/66628#discussion_r3751495474
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadata.java:
##########
@@ -646,7 +646,8 @@ private Table loadTable(ConnectorSession session,
IcebergTableHandle handle) {
try {
return context.executeAuthenticated(() ->
resolveTableForRead(session, handle));
} catch (Exception e) {
- throw new RuntimeException("Failed to load table, error message
is:" + e.getMessage(), e);
+ throw IcebergExceptionUtils.wrapTableLoadFailure(
Review Comment:
The new normalizer is still downstream of production failures.
`getTableHandle()` calls `catalogOps.tableExists()`; Glue, Hadoop, JDBC, and S3
Tables use a default existence path that loads the table, so a missing metadata
JSON is wrapped as `Failed to check table exist...` before this helper is
reached. HMS/REST get farther, but `SHOW PARTITIONS` calls
`listPartitionNames() -> resolveTableForRead()` directly and keeps its generic
wrapper. The new tests construct handles directly, so they miss both real entry
paths. Please normalize at the shared handle/load boundary while preserving
`NoSuchTableException`, audit direct readers, and cover full
handle-resolution/SHOW paths with Iceberg `NotFoundException` and a deeper
cause.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/scan/PluginDrivenScanNode.java:
##########
@@ -1289,23 +1292,32 @@ void checkSysTableScanConstraints() throws
UserException {
String sysTableName = sysTableName();
if (scanParams.incrementalRead()) {
if (!sysTableSupportsScanParam(p ->
p.supportsSystemTableIncrementalRead(sysTableName))) {
- throw new UserException("Plugin system table '" +
sysTableName
+ throw new UserException(connectorName + " system table '"
+ sysTableName
+ "' does not support INCR scan params.");
}
} else if (scanParams.isOptions()) {
if (!sysTableSupportsScanParam(p ->
p.supportsSystemTableOptions(sysTableName))) {
- throw new UserException("Plugin system table '" +
sysTableName
+ throw new UserException(connectorName + " system table '"
+ sysTableName
+ "' does not support OPTIONS scan params.");
}
} else if (!timeTravelSupported) {
- throw new UserException("Plugin system tables do not support
scan params.");
+ throw new UserException(connectorName + " system tables do not
support scan params.");
}
}
if (getQueryTableSnapshot() != null && !timeTravelSupported) {
- throw new UserException("Plugin system tables do not support time
travel.");
+ throw new UserException(connectorName + " system tables do not
support time travel.");
}
}
+ private String connectorDisplayName() throws UserException {
+ String engine = getTargetTable().getEngine();
+ // The connector identity is part of the user-facing compatibility
contract; the generic plugin layer
+ // must not make equivalent connector errors differ according to which
execution path rejected a scan.
+ return engine == null || engine.isEmpty()
Review Comment:
`getEngine()` already returns the connector-owned `displayEngineName`, which
the SPI and existing engine tests preserve verbatim. Uppercasing the first
UTF-16 unit here corrupts valid custom spellings (`iRODS` -> `IRODS`,
`eBayLake` -> `EBayLake`) in every new sys-table rejection. Please keep the
nonempty value exact and reword the sentence if lowercase-at-start is
undesirable; add a mixed-case name test and exact assertions for the INCR,
OPTIONS, generic-param, and time-travel branches.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/scan/PluginDrivenScanNode.java:
##########
@@ -221,9 +221,11 @@ public static PluginDrivenScanNode create(PlanNodeId id,
TupleDescriptor desc,
// PluginDrivenSysExternalTable the override returns the connector's
SYSTEM handle (carrying
// sysTableName + forceJni), so the scan path threads force-JNI
correctly for binlog/audit_log.
ConnectorTableHandle handle =
table.resolveConnectorTableHandle(session, metadata)
+ // A missing handle means the remotely resolved table
disappeared; keep connector internals out
+ // of the user-facing error so redirected tables retain the
normal table-not-found contract.
.orElseThrow(() -> new RuntimeException(
- "Table handle not found for plugin-driven table: " +
dbName + "."
- + table.getRemoteName()));
+ "Table '" + catalog.getName() + "." + dbName + "." +
table.getRemoteName()
Review Comment:
This still is not the normal user-facing table-not-found path. The
identifier combines the local catalog with `db.getRemoteName()` and
`table.getRemoteName()`, so an identifier-mapped query such as
`cat.local_db.local_tbl` can fail as `cat.REMOTE_DB.REMOTE_TBL` (the new test
actually creates local `tbl` but asserts remote `dropped_table`). It is also a
plain `RuntimeException`: `EXPLAIN` invokes this through `ExplainCommand`,
where `StmtExecutor` routes non-`UserException` failures through the internal
`ERR_UNKNOWN_ERROR` arm. Please translate this to a user/analysis
table-not-found failure before the EXPLAIN command boundary and render `catalog
+ local db + table.getName()`; keep remote coordinates only for connector
calls/logging, with mapped normal/sys-table tests.
--
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]
