github-actions[bot] commented on code in PR #66247:
URL: https://github.com/apache/doris/pull/66247#discussion_r3680041637
##########
be/src/format_v2/jni/jni_table_reader.cpp:
##########
@@ -511,13 +513,21 @@ void JniTableReader::_prepare_jni_scanner_schema() {
required_fields.push_back(column.java_name);
column_types.push_back(
JniDataBridge::get_jni_type_with_different_string(column.transfer_type));
+ encoded_column_types.push_back(
Review Comment:
[P2] Avoid encoding Paimon-only schema fields for every V2 connector
`_prepare_jni_scanner_schema()` runs once per split in the shared V2 base,
so this recursively walks every column type again, Base64-encodes both lists,
and copies two extra strings into the Java parameter map for JDBC, Hudi, Trino,
Iceberg system-table, and MaxCompute scans as well. A repository-wide consumer
search finds these paired fields used only by `PaimonJniScanner`; the other
scanners discard the work. Please make encoded-schema publication a Paimon
capability/override (or add the fields in `PaimonJniReader`) so unrelated
connectors do not pay this per-split cost.
##########
regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy:
##########
@@ -0,0 +1,224 @@
+// 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.
+
+suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") {
+ String enabled = context.config.otherConfigs.get("enablePaimonTest")
+ if (enabled == null || !enabled.equalsIgnoreCase("true")) {
+ logger.info("disable paimon test")
+ return
+ }
+
+ String minioPort = context.config.otherConfigs.get("iceberg_minio_port")
+ String externalEnvIp = context.config.otherConfigs.get("externalEnvIp")
+ String catalogName = "test_paimon_jni_reader_guardrails"
+ String physicalCatalogName = "test_paimon_jni_reader_physical_guardrails"
+ String dbName = "paimon_jni_guardrails_db"
+ def scannerV2Rows = sql "show variables like 'enable_file_scanner_v2'"
+ String originalScannerV2 = scannerV2Rows[0][1]
+
+ def catalogDdl = { String name, String extraProperty ->
+ return """
+ create catalog ${name} properties (
+ 'type'='paimon',
+ 'warehouse'='s3://warehouse/wh',
+ 's3.endpoint'='http://${externalEnvIp}:${minioPort}',
+ 's3.access_key'='admin',
+ 's3.secret_key'='password',
+ 's3.path.style.access'='true'
+ ${extraProperty}
+ )
+ """
+ }
+
+ for (def invalid : [
+ ["invalid_batch", ", 'paimon.table-option.read.batch-size'='0'",
"read.batch-size"],
+ ["unsafe_branch", ", 'paimon.table-option.branch'='archive'",
"branch"]
+ ]) {
+ String invalidCatalog = "${catalogName}_${invalid[0]}"
+ sql "drop catalog if exists ${invalidCatalog}"
+ try {
+ test {
+ sql(catalogDdl(invalidCatalog, invalid[1]))
+ exception invalid[2]
+ }
+ } finally {
+ sql "drop catalog if exists ${invalidCatalog}"
+ }
+ }
+
+ sql "drop catalog if exists ${catalogName}"
+ sql(catalogDdl(catalogName, """
+ , 'paimon.table-option.read.batch-size'='1024'
+ , 'paimon.table-option.file-reader-async-threshold'='16 MB'
+ """))
+
+ try {
+ spark_paimon_multi """
+ create database if not exists paimon.${dbName};
+ drop table if exists paimon.${dbName}.quoted_reader_options;
+ create table paimon.${dbName}.quoted_reader_options (
+ id int,
+ `region,code` string,
+ `hash#name` string,
+ `display name` string,
+ `地区 名` string,
+ `nested#value`
struct<`hash#name`:string,`region,code`:string,`colon:name`:string>
+ ) using paimon
+ tblproperties ('file.format'='parquet', 'read.batch-size'='512');
+ insert into paimon.${dbName}.quoted_reader_options values
+ (1, 'east,01', 'hash-one', 'first row', '华东',
+ named_struct('hash#name', 'nested-one', 'region,code',
'east,01', 'colon:name', 'a:1')),
+ (2, 'west,02', 'hash-two', 'second row', '华西',
+ named_struct('hash#name', 'nested-two', 'region,code',
'west,02', 'colon:name', 'b:2'));
+ drop table if exists paimon.${dbName}.unsafe_physical_batch;
+ create table paimon.${dbName}.unsafe_physical_batch (id int) using
paimon
+ tblproperties ('read.batch-size'='0');
+ insert into paimon.${dbName}.unsafe_physical_batch values (1);
+ drop table if exists paimon.${dbName}.unsafe_physical_manifest;
+ create table paimon.${dbName}.unsafe_physical_manifest (id int,
part int) using paimon
+ partitioned by (part)
+ tblproperties ('scan.manifest.parallelism'='0');
+ insert into paimon.${dbName}.unsafe_physical_manifest values (1,
10);
+ drop table if exists paimon.${dbName}.empty_identifier;
+ create table paimon.${dbName}.empty_identifier (`` string) using
paimon;
+ insert into paimon.${dbName}.empty_identifier values
('empty-name');
+ """
+
+ sql "switch ${catalogName}"
+ sql "use ${dbName}"
+ sql "set force_jni_scanner=true"
+
+ test {
Review Comment:
[P2] Assert the values returned by the JNI guardrail queries
These success-path blocks only check that the SQL does not throw; they do
not record or assert any rows. The suite would still pass if the encoded nested
fields came back as NULL/wrong values, if the empty identifier were misbound,
or if the two relation-local scans/join produced no rows. Since these are the
end-to-end checks for the V1/V2 framing and option-isolation changes, please
use named `qt_`/`order_qt_` cases with a generated `.out` for both scanner
modes and the safe-override queries; keep `test { ... exception ... }` for the
negative cases.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java:
##########
@@ -178,6 +178,19 @@ private PaimonSnapshotCacheValue
getPaimonSnapshotCacheValue(Optional<TableSnaps
"Failed to get Paimon snapshot: " + (e.getMessage() ==
null ? "unknown cause" : e.getMessage()),
e);
}
+ } else if (scanParams.isPresent() && scanParams.get().isOptions()) {
+ Table baseTable = getBasePaimonTable();
+ Map<String, String> resolvedOptions =
scanParams.get().getOrResolveMapParams(
+ options -> PaimonScanParams.resolveOptions(baseTable,
options));
+ Table effectiveTable = PaimonScanParams.applyOptions(baseTable,
resolvedOptions);
+ if (PaimonScanParams.hasOnlyReaderOptions(resolvedOptions)) {
+ // Reader tuning cannot change snapshot metadata. Reuse the
memoized projection so
+ // a per-query batch-size change does not enumerate every
partition again.
+ return PaimonUtils.getLatestSnapshotCacheValue(this);
Review Comment:
[P1] Do not revalidate the raw reader setting on the memoized path
A partitioned table with physical `read.batch-size=0` and
`@options('read.batch-size'='4096')` is safe after `applyOptions` above, but
this returns a memoized projection whose supplier captured the raw physical
table. `PaimonLatestSnapshotProjectionLoader` then calls
`PaimonPartitionInfoLoader`, which revalidates that raw handle before
enumerating partitions and rejects the zero value. Reader batch size does not
participate in partition metadata, so only partitioned tables lose the
documented safe relation override here. Please keep reader-option validation on
the final effective handle without revalidating the raw reader value during
memoized metadata loading, and cover the composed
partitioned/unsafe-physical/safe-relation case.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSysExternalTable.java:
##########
@@ -117,15 +120,24 @@ private static long generateSysTableId(long
sourceTableId, String sysTableType)
* Note: system tables currently ignore snapshot semantics.
*/
public Table getSysPaimonTable() {
+ validateEffectiveDataTable(null);
Review Comment:
[P1] Preserve the safe system-table override during descriptor serialization
This no-argument path is still reached after the relation-specific path
succeeds: `DescriptorToThriftConverter` calls
`PaimonSysExternalTable.toThrift()`, which calls the cached no-arg
`getFullSchema()` and then `buildFullSchema() -> getSysPaimonTable()`. For a
physical `scan.manifest.parallelism=0` table queried as
`$partitions@options('scan.manifest.parallelism'='1')`, schema binding and
`getProcessedTable()` use the safe copied data handle, but descriptor
serialization comes back here with `null` and rejects the raw hidden handle.
This is a later boundary than the existing hidden-wrapper thread. Please keep
schema/descriptor construction non-planning (or carry the effective schema
already bound for this relation), and extend the safe-override test through
descriptor serialization.
--
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]
