github-actions[bot] commented on code in PR #66247:
URL: https://github.com/apache/doris/pull/66247#discussion_r3676310132
##########
be/src/format_v2/jni/jni_table_reader.cpp:
##########
@@ -517,6 +529,9 @@ void JniTableReader::_prepare_jni_scanner_schema() {
{column.transfer_type->create_column(), column.transfer_type,
column.java_name});
}
_scanner_params["required_fields"] = join(required_fields, ",");
+ // Preserve the legacy field during rolling upgrades, while new scanners
use independently
+ // encoded identifiers so commas and other legal identifier characters
cannot alter framing.
+ _scanner_params["required_fields_base64"] =
encode_jni_required_fields(required_fields);
_scanner_params["columns_types"] = join(column_types, "#");
Review Comment:
[P1] Make the type-list framing delimiter-safe too
The new field payload is safe, but the adjacent `columns_types` payload is
still `#`-joined. `JniDataBridge::get_jni_type_with_different_string()` embeds
STRUCT child names verbatim, so a legal Paimon column with type
`struct<hash#name:string>` produces one required field but Java splits its type
into two tokens and fails the same cardinality check. (Comma/colon-bearing
child names also collide with the STRUCT parser.) The regression's top-level
scalar `hash#name` never enters `columns_types`, so it misses this case. Please
frame/decode each complete type descriptor safely and add nested quoted-name
coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractPaimonProperties.java:
##########
@@ -188,44 +172,14 @@ private Map<String, String> extractTableOptions() {
}
private void validateTableOption(String key, String value) {
- ConfigOption<?> option = SUPPORTED_TABLE_OPTIONS.find(key);
- if (option == null) {
- throw new IllegalArgumentException("Unsupported Paimon table
option '" + key
- + "' for the bundled Paimon version");
- }
-
try {
- new Options(Collections.singletonMap(key, value)).get(option);
+ PaimonReaderOptions.validate(key, value);
Review Comment:
[P1] Preserve persisted catalogs during the allowlist migration
Catalogs created before this change can already contain other Paimon
`CoreOptions` under `paimon.table-option.*` (the changed tests use
`write.batch-size` and `file.compression.per.level` as formerly accepted
examples). On restart only the raw `CatalogProperty.properties` map is
restored; the first `getMetastoreProperties()` rebuild runs this validator and
now rejects the legacy key, so that catalog can no longer initialize. Skipping
validation during edit-log replay does not avoid the lazy reconstruction, and
ALTER has no unset path. Please provide a load-time migration/compatibility
path while rejecting new CREATE/ALTER use, and add image/edit-log upgrade
coverage.
##########
be/src/format_v2/jni/jni_table_reader.cpp:
##########
@@ -517,6 +529,9 @@ void JniTableReader::_prepare_jni_scanner_schema() {
{column.transfer_type->create_column(), column.transfer_type,
column.java_name});
}
_scanner_params["required_fields"] = join(required_fields, ",");
+ // Preserve the legacy field during rolling upgrades, while new scanners
use independently
+ // encoded identifiers so commas and other legal identifier characters
cannot alter framing.
+ _scanner_params["required_fields_base64"] =
encode_jni_required_fields(required_fields);
Review Comment:
[P1] Cover the still-supported legacy Paimon JNI producer
`required_fields_base64` is emitted only by `FileScannerV2`, but
`enable_file_scanner_v2=false` (and load routing) still constructs
`be/src/format/table/paimon_jni_reader.cpp`. That producer sends only
comma-joined `required_fields`, so this scanner takes the legacy fallback and
turns one legal `region,code` column into two field names while `columns_types`
still has one entry; the constructor rejects the query. The new regression
leaves Scanner V2 enabled, so it cannot catch this path. Please emit the
encoded field list from the legacy Paimon reader as well and cover the V1 route.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java:
##########
@@ -656,11 +656,17 @@ public void replayAlterCatalogProps(CatalogLog log,
Map<String, String> oldPrope
if (!isReplay) {
try {
((ExternalCatalog) catalog).checkProperties();
- } catch (DdlException ddlException) {
+ } catch (Exception validationException) {
Review Comment:
[P1] Validate before publishing the candidate property map
`tryModifyCatalogProps` updates the live property map and clears its lazy
caches before validation, while normal catalog lookup/initialization does not
take this `CatalogMgr` lock. A concurrent first query can therefore run
`PaimonExternalCatalog.initLocalObjectsImpl()` against the tentative values and
retain that derived `paimonProperties` object. If validation then rejects (for
example, a valid new reader option plus an invalid refresh interval), this
catch restores only `CatalogProperty`; the catalog stays initialized with
unjournaled values. Please validate a detached snapshot before publication (or
mutually exclude every property consumer) and add a latch-based
ALTER/initialization race test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java:
##########
@@ -136,8 +136,7 @@ public Table getPaimonTable(NameMapping nameMapping, String
branch, String query
}
return executionAuthenticator.execute(() -> {
Table table = catalog.getTable(identifier);
- Map<String, String> tableOptions =
-
paimonProperties.getTableOptionsForCopy(table.options());
+ Map<String, String> tableOptions =
paimonProperties.getTableOptionsForCopy();
Review Comment:
[P1] Validate the effective physical table options
These checks cover Doris catalog/relation overrides, but
`catalog.getTable(identifier)` can already contain unsafe physical values. With
no catalog override this returns the table unchanged; with an override,
Paimon's `Table.copy` retains every omitted physical key. Thus a physical
`scan.manifest.parallelism=0` still reaches `newScan().plan()` and blocks on
Paimon's zero-permit wrapper (and a huge value still replaces its JVM-static
pool); physical `read.batch-size=0` likewise reaches JNI unchanged. Please
validate the effective merged table before planning/serialization and add a
fixture whose unsafe value comes from the physical Paimon table.
--
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]
