github-actions[bot] commented on code in PR #65867:
URL: https://github.com/apache/doris/pull/65867#discussion_r3652746290
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -190,6 +196,142 @@ protected void doInitialize() throws UserException {
}
}
+ /**
+ * Build the Paimon table object that is serialized to the BE.
+ *
+ * <p>Every table loaded from a metastore-backed Paimon catalog (HMS /
DLF) carries a Paimon
+ * {@code HiveCatalogLoader} in its {@link
org.apache.paimon.table.CatalogEnvironment}. The BE
+ * only reads — via FE-resolved splits and the object store — and never
needs the catalog, yet
+ * deserializing that loader forces the whole Hive metastore stack onto
the BE classpath:
+ * {@code HiveConf}, the metastore API, and, when a system table resolves
its latest snapshot,
+ * even the metastore client (DLF's {@code ProxyMetaStoreClient} and its
REST stack). So we
+ * serialize a catalog-less table to the BE:
+ * <ul>
+ * <li>data table: drop the catalog loader. A {@link FileStoreTable} is
fully defined by
+ * fileIO / location / schema / catalogEnvironment, and its dynamic
options (time travel,
+ * incremental) are merged into the schema by {@code copy(...)}, so
rebuilding from
+ * fileIO / location / schema preserves everything except the
catalog loader.</li>
+ * <li>system table (e.g. {@code $snapshots}): rebuild it over a
catalog-less data table so
+ * {@code SnapshotManager#latestSnapshotId} lists the snapshot
directory on the filesystem
+ * instead of calling the metastore. The base table is the one the
FE-side wrapper was
+ * built over, and for the system tables that re-plan on the BE
({@link
+ * #replansOnBackend}) what the catalog would have done there is
done here instead: see
+ * {@link #authorizeDeferredScan} and {@link #pinCatalogSnapshot}.
Every other system
+ * table reads the splits the FE already planned, so it is handed
over untouched.</li>
+ * </ul>
+ */
+ private Table getPaimonTableForBackend() {
+ Table paimonTable = source.getPaimonTable();
+ if (paimonTable instanceof FileStoreTable) {
+ return dropCatalogLoader((FileStoreTable) paimonTable);
+ }
+ if (!(source.getExternalTable() instanceof PaimonSysExternalTable)) {
+ return paimonTable;
+ }
+ PaimonSysExternalTable sysTable = (PaimonSysExternalTable)
source.getExternalTable();
+ // The very same base table the FE-side wrapper was built over, so
that the BE never sees a
+ // different schema generation than the one this query was planned
with.
+ FileStoreTable dataTable = sysTable.getSysBaseTable();
+ if (dataTable == null) {
+ return paimonTable;
+ }
+ String sysTableType = sysTable.getSysTableType();
+ if (PAIMON_FILES_SYSTEM_TABLE_TYPE.equalsIgnoreCase(sysTableType)) {
+ authorizeDeferredScan(dataTable);
+ }
+ FileStoreTable baseForBackend = dropCatalogLoader(dataTable);
+ if (replansOnBackend(sysTableType)) {
+ baseForBackend = pinCatalogSnapshot(baseForBackend, dataTable);
+ }
+ Table catalogLessSysTable = SystemTableLoader.load(sysTableType,
baseForBackend);
+ return catalogLessSysTable == null ? paimonTable : catalogLessSysTable;
+ }
+
+ /**
+ * {@code $files} and {@code $partitions} are the only system tables that
do not read what the FE
+ * planned: they carry partition-level splits and call {@code
FileStoreTable#newScan} again on
+ * the BE ({@code FilesTable.FilesSplit#splits},
+ * {@code PartitionsTable.PartitionsRead#createReader}).
+ */
+ private static boolean replansOnBackend(String sysTableType) {
+ return PAIMON_FILES_SYSTEM_TABLE_TYPE.equalsIgnoreCase(sysTableType)
+ ||
PAIMON_PARTITIONS_SYSTEM_TABLE_TYPE.equalsIgnoreCase(sysTableType);
+ }
+
+ /**
+ * {@code $files} plans only partition-level splits on the FE and re-plans
the base table on the
+ * BE through {@code DataTableScan#plan()} ({@code
FilesTable.FilesRead#createReader}). That
+ * deferred plan normally authorizes itself through the catalog loader
+ * ({@code CatalogEnvironment#tableQueryAuth} -> {@code
Catalog#authTableQuery}); once the
+ * loader is dropped it silently allows everything. So authorize here,
while the loader is still
+ * around. Paimon discards the predicates the call returns (row level
access control is a TODO in
+ * {@code AbstractDataTableScan#authQuery}), so running it on the FE loses
nothing.
+ *
+ * <p>Only {@code $files} may do this: {@code auth(null)} means "every
column" to
+ * {@code Catalog#authTableQuery}, and the system tables that keep
planning on the FE
+ * ({@code $ro}, {@code $row_tracking}, {@code $audit_log}, {@code
$binlog}) already authorize
+ * themselves through {@code DataTableBatchScan} with the slot projection
the query really reads.
+ * Authorizing those again for every column would reject a user allowed to
read only some of the
+ * base columns. {@code $partitions} never reaches {@code plan()} on
either side, so it has no
+ * authorization to transfer.
+ */
+ private static void authorizeDeferredScan(FileStoreTable dataTable) {
+ CoreOptions options = dataTable.coreOptions();
+ if (options.queryAuthEnabled()) {
+ dataTable.catalogEnvironment().tableQueryAuth(options).auth(null);
+ }
+ }
+
+ /**
+ * For catalogs that manage versions themselves (Paimon REST / DLF REST)
the committed snapshot
+ * is the one the catalog points at, not the newest file in the snapshot
directory: Paimon
+ * publishes the snapshot file before the pointer moves, and a rollback
leaves newer files
+ * behind. Without the catalog loader {@code SnapshotManager} falls back
to listing that
+ * directory, so the BE could plan on a snapshot the catalog has not
published while the FE
+ * planned on the previous one. Pin the catalog-visible snapshot instead.
+ *
+ * <p>Only for {@link #replansOnBackend} system tables. {@code
PaimonJniScanner#initTable}
+ * unconditionally calls {@code table.copy(table.options())}, and every
Paimon system-table
+ * wrapper delegates {@code copy} to {@code FileStoreTable#copy}, which
time-travels the schema
+ * to {@code scan.snapshot-id}. For a wrapper whose row type follows the
base table
+ * ({@code $ro}, {@code $row_tracking}, {@code $audit_log}, {@code
$binlog}) that would rewind
+ * the BE schema: a column added after the latest snapshot is planned by
the FE and then
+ * rejected by {@code PaimonJniScanner#getProjected} with "RequiredField
... not found in
+ * schema". {@code $files} / {@code $partitions} have a fixed row type, so
the pin is safe there.
+ *
+ * <p>Known gap: {@code scan.snapshot-id} only bounds plans that go through
+ * {@code DataTableScan}. {@code $snapshots} ({@code
SnapshotManager#snapshotsWithinRange}) and
+ * {@code $buckets} ({@code SnapshotReader#bucketEntries}) ignore it, so
on the BE they observe
+ * the snapshot directory rather than the catalog's pointer. Both are
read-only metadata tables,
+ * so this shows up only as a transient extra row inside the publication
window of a
+ * version-managed catalog.
+ */
+ private static FileStoreTable pinCatalogSnapshot(FileStoreTable
catalogLessTable, FileStoreTable dataTable) {
+ if (!dataTable.catalogEnvironment().supportsVersionManagement()) {
+ return catalogLessTable;
+ }
+ Long snapshotId = dataTable.snapshotManager().latestSnapshotId();
+ if (snapshotId == null) {
+ return catalogLessTable;
+ }
+ // Without time travel: pin which snapshot the BE plans on, leave
schema resolution alone.
+ return catalogLessTable.copyWithoutTimeTravel(
Review Comment:
[P1] Preserve the fallback branch's catalog snapshot bound
For a version-managed table with `scan.fallback-branch`, this pins only the
main branch after `dropCatalogLoader` has already rebuilt both branches with
empty catalog environments. Paimon's
`FallbackReadFileStoreTable.copyWithoutTimeTravel` translates that main
snapshot to a fallback snapshot via
`fallback.snapshotManager().earlierOrEqualTimeMills(...)`; that lookup now sees
filesystem-latest rather than the fallback branch's catalog pointer. During
publication or after rollback, a `$partitions` BE reader can therefore merge
entries from an unpublished or rollback-retained fallback snapshot even though
the main branch is correctly pinned. Please resolve and carry both branches'
catalog-visible bounds before removing the loaders, and cover fallback
publication/rollback cases.
##########
build.sh:
##########
@@ -1239,6 +1239,31 @@ EOF
fi
done
+ # Guard the Paimon FileIO SPI classloader boundary on the built artifacts.
The FileIOLoader
+ # interface (paimon-common) and every provider implementing it must sit in
the same jar:
+ # JniScannerClassLoader delegates parent-first, so a provider left on the
shared
+ # preload-extensions (JVM app) classpath cannot resolve the child-only
interface and
+ # ServiceLoader discovery aborts at runtime with "NoClassDefFoundError:
FileIOLoader". Unit
+ # tests all run in one classloader and cannot reproduce that, so assert it
on the packages.
+
PAIMON_SCANNER_JAR="${BE_JAVA_EXTENSIONS_DIR}/paimon-scanner/paimon-scanner-jar-with-dependencies.jar"
+
PRELOAD_JAR="${BE_JAVA_EXTENSIONS_DIR}/preload-extensions/preload-extensions-jar-with-dependencies.jar"
+ if [[ -f "${PAIMON_SCANNER_JAR}" ]]; then
+ FILE_IO_SERVICES="$(unzip -p "${PAIMON_SCANNER_JAR}"
META-INF/services/org.apache.paimon.fs.FileIOLoader)"
+ for loader in org.apache.paimon.s3.S3Loader
org.apache.paimon.jindo.JindoLoader; do
+ if ! echo "${FILE_IO_SERVICES}" | grep -q -x "${loader}"; then
+ echo "ERROR: ${loader} is missing from
META-INF/services/org.apache.paimon.fs.FileIOLoader"
+ echo " in ${PAIMON_SCANNER_JAR}. Paimon object-store
reads on BE would fail;"
+ echo " keep the FileIO plugins bundled in
paimon-scanner."
+ exit 1
+ fi
+ done
+ fi
+ if [[ -f "${PRELOAD_JAR}" ]] && unzip -l "${PRELOAD_JAR}" | grep -q -E
'org/apache/paimon/(s3|jindo)/'; then
Review Comment:
[P1] Make this guard consume the full archive listing
`build.sh` runs with `set -o pipefail`, so when `grep -q` finds an early
provider entry in this large fat jar, it closes the pipe and `unzip` exits with
SIGPIPE (141). The combined condition then evaluates false and skips the error
body, letting the exact forbidden parent-loaded provider pass. I reproduced
this with the same pipeline on a 5,096-entry repository archive: the matching
entry was present, but the `if` selected its false branch with status 141. This
is distinct from the earlier packaging-coverage thread because the newly added
guard itself is unreliable even when it runs. Please consume the complete
listing (for example, use non-quiet `grep` with output redirected) or capture
it before matching.
--
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]
