github-actions[bot] commented on code in PR #66247:
URL: https://github.com/apache/doris/pull/66247#discussion_r3684457161
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/StatementContext.java:
##########
@@ -1131,7 +1131,7 @@ private static String
versionKeyOf(Optional<TableSnapshot> tableSnapshot,
}
if (scanParams != null && scanParams.isPresent()) {
TableScanParams sp = scanParams.get();
-
key.append("p:").append(sp.getParamType()).append(':').append(sp.getMapParams())
+ key.append("p:").append(sp.getParamType()).append(':').append(new
TreeMap<>(sp.getMapParams()))
Review Comment:
[P1] Make the selector key structurally injective
Sorting fixes iteration-order drift, but `Map.toString()` is still
delimiter-ambiguous. For example, the valid Paimon selector maps
`{'scan.tag-name'='v1, source.split.target-size=1 MB'}` and
`{'scan.tag-name'='v1', 'source.split.target-size'='1 MB'}` both render as
`{scan.tag-name=v1, source.split.target-size=1 MB}`. If two aliases use those
selectors, `loadSnapshots` stores only the first entry and both scan-time
lookups return it, so one alias silently reads the wrong tag/snapshot depending
on bind order. Please use a structural key (or a length-prefixed encoding of
the param type, sorted entries, and list values) and add both collision and
reversed-entry order tests.
##########
fe/fe-connector/pom.xml:
##########
@@ -55,7 +55,7 @@ under the License.
of the latter two means bumping this property as well (and
fe-extension-spi means bumping
all four families).
-->
- <connector.plugin.api.version>1.0</connector.plugin.api.version>
+ <connector.plugin.api.version>2.0</connector.plugin.api.version>
Review Comment:
[P1] Enforce the new major for classpath providers too
This bump reaches `DirectoryPluginRuntimeManager`, but
`ConnectorPluginManager.loadBuiltins()` registers every classpath
`ServiceLoader` provider without consulting `API_VERSION_GATE` or its manifest.
Because connector SPI packages are parent-first, an API-1 provider used in the
documented embedded/classpath mode still links against this API-2 interface and
silently inherits the new `validatePropertiesForUpdate` default—the exact
compatibility failure the major bump is meant to prevent. Please apply the same
fail-closed manifest-major check to classpath discovery (or explicitly disallow
and version-lock that mode), and cover an API-1 classpath provider under an
API-2 kernel.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonConnectorMetadata.java:
##########
@@ -1276,21 +1289,26 @@ private List<ConnectorPartitionInfo>
collectPartitions(PaimonTableHandle paimonH
return Collections.emptyList();
}
- // Partition enumeration is intentionally BASE-only: branch /
time-travel reads carry EMPTY
- // partition info (legacy PaimonPartitionInfo.EMPTY) and never reach
this path, so for the
- // (non-branch) handles that do, resolveTable returns the base table
and the base-Identifier
- // listing below is consistent. (A branch handle would otherwise mix
branch schema metadata
- // here with the base partition list — but that combination does not
occur by design.)
- Table table = resolveTable(paimonHandle);
+ Table resolvedTable = resolveTable(paimonHandle);
+ boolean optionsPin =
PaimonScanParams.isOptionsPin(paimonHandle.getScanOptions());
+ Table table;
+ if (optionsPin) {
+ table = PaimonScanParams.applyOptions(resolvedTable,
paimonHandle.getScanOptions());
+ } else {
+ // Partition projection never opens a data reader, so reader-only
settings must not
+ // invalidate metadata that a later relation-scoped override can
make safe.
+ PaimonReaderOptions.validateEffectivePlanningTable(resolvedTable);
+ table = resolvedTable;
+ }
Identifier identifier = Identifier.create(
paimonHandle.getDatabaseName(), paimonHandle.getTableName());
- // M-11: wrap the remote listPartitions in executeAuthenticated
(D-052), mirroring legacy
- // PaimonExternalCatalog.getPaimonPartitions which ran it inside
executionAuthenticator.execute
- // and swallowed TableNotExistException INSIDE the wrap (Kerberos
UGI.doAs would otherwise wrap
- // the checked exception, so it must be caught inside).
List<Partition> paimonPartitions;
try {
paimonPartitions = context.executeAuthenticated(() -> {
+ if (optionsPin) {
Review Comment:
[P1] Preserve catalog reader policy during partition enumeration
`resolvedTable` already includes `paimon.table-option.*` via
`CatalogBackedPaimonCatalogOps.getTable`, but this plain-relation branch
discards it by calling `catalog.listPartitions(identifier)`, which reloads the
raw physical table. A partitioned table with physical
`scan.manifest.parallelism=0` and catalog override
`paimon.table-option.scan.manifest.parallelism=1` therefore passes the
effective-table validation above and then enumerates manifests with the unsafe
raw value, potentially blocking before split planning. The changed P0 fixture
creates exactly this combination but never queries it under the overriding
catalog. Please enumerate from the already resolved effective table (or make
the seam accept that exact table) for catalog-scoped overrides too, and add the
missing physical-unsafe/catalog-safe partition test.
--
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]
