github-actions[bot] commented on code in PR #67904:
URL: https://github.com/apache/doris/pull/67904#discussion_r3998709927
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonConnectorMetadata.java:
##########
@@ -733,9 +734,9 @@ public Optional<ConnectorMvccSnapshot> resolveTimeTravel(
// latestSnapshot() only).
Table branchTable =
resolveTable(paimonHandle.withBranch(branchName));
long snapshotId =
catalogOps.latestSnapshotId(branchTable).orElse(-1L);
- long schemaId = snapshotId < 0
- ? -1L
- : catalogOps.snapshotSchemaId(branchTable,
snapshotId).orElse(-1L);
+ // A schema-only ALTER advances the branch schema without
creating a data snapshot.
+ // Keep the data fence but bind the branch's current schema
instead of that snapshot's old schema.
+ long schemaId = -1L;
Review Comment:
[P1] Enforce the recorded branch data fence. This code records the branch
latest snapshot id, but applySnapshot handles the branch sentinel first and
returns withBranch(branch) without carrying that id; withBranch clears the
transient table, so split planning and JNI serialization reload the then-latest
branch with no scan.snapshot-id, and an empty branch has no empty marker. A
commit between binding and planning can therefore change the rows observed by
the statement. Please carry branch identity together with the recorded
positive/empty fence while retaining the current branch schema, and add a
mutation test between resolution and planning.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java:
##########
@@ -346,7 +346,13 @@ Table resolveScanTable(PaimonTableHandle paimonHandle) {
Map<String, String> scanOptions = paimonHandle.getScanOptions();
Table finalTable = table;
if (scanOptions != null && !scanOptions.isEmpty()) {
- if (PaimonScanParams.isOptionsPin(scanOptions)) {
+ if (table instanceof FileStoreTable
+ && PaimonScanParams.preservesBoundSchema(scanOptions)) {
Review Comment:
[P1] Preserve the bound schema for schema-derived system wrappers too. A
selector-free statement-fenced @options carries this marker, but
$ro/$audit_log/$binlog/$row_tracking are wrappers rather than FileStoreTable
instances, so they miss this arm and fall through to applyOptions/Table.copy,
rewinding a fresh S2 wrapper to the data snapshot S1 schema. Metadata binds S1,
the scan path can rebuild S2 from the preserve-aware source, and
tableForBackend serializes S1 again. Schema-only add/rename queries therefore
fail or disagree even without a stale cache. Please apply the provenance to the
source before building the wrapper in every binding/planning/backend path and
add a supported system-alias mutation test.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java:
##########
@@ -346,7 +346,13 @@ Table resolveScanTable(PaimonTableHandle paimonHandle) {
Map<String, String> scanOptions = paimonHandle.getScanOptions();
Table finalTable = table;
if (scanOptions != null && !scanOptions.isEmpty()) {
- if (PaimonScanParams.isOptionsPin(scanOptions)) {
+ if (table instanceof FileStoreTable
+ && PaimonScanParams.preservesBoundSchema(scanOptions)) {
+ // A statement fence owns data visibility, not schema time
travel. Reusing Table.copy
+ // here would roll schema-only ALTERs back to the data
snapshot's older schema.
+ finalTable = PaimonScanParams.applyOptionsWithoutTimeTravel(
+ (FileStoreTable) table, scanOptions);
Review Comment:
[P1] Carry the exact schema Doris bound, not merely a preserve boolean.
getTableSchema deliberately reads schemaManager().latest() because Paimon
cached Table.rowType() is frozen, but copyWithoutTimeTravel keeps the schema
embedded in this table object. After warming S1 and applying an external
schema-only ALTER to S2, analysis binds S2 while this path still plans and
serializes S1; @options fails the missing-column check and JNI cannot resolve
the requested S2 field. Please carry the exact bound schema generation into
this chokepoint and add a warm-cache mutation test covering native planning and
serialized/JNI reads; the new tests reload an uncached table after ALTER and
mask this case.
##########
regression-test/suites/external_table_p0/paimon/test_paimon_schema_only_snapshot_precision.groovy:
##########
@@ -0,0 +1,124 @@
+// 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_schema_only_snapshot_precision", "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_schema_only_snapshot_precision"
+ String dbName = "paimon_schema_only_snapshot_precision_db"
+ String tableName = "schema_only_timeline"
+ String branchName = "schema_only_branch"
+
+ def latestSnapshotId = {
+ List<List<Object>> rows = spark_paimon """
+ select snapshot_id
+ from paimon.${dbName}.`${tableName}\$snapshots`
+ order by snapshot_id desc
+ limit 1
+ """
+ assertEquals(1, rows.size())
+ return rows[0][0].toString()
+ }
+
+ sql """drop catalog if exists ${catalogName}"""
+ sql """
+ create catalog ${catalogName} 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',
+ 'meta.cache.paimon.table.ttl-second'='0'
+ )
+ """
+
+ try {
+ spark_paimon_multi """
+ create database if not exists paimon.${dbName};
+ drop table if exists paimon.${dbName}.${tableName};
+ create table paimon.${dbName}.${tableName} (
+ id int,
+ old_name string,
+ event_time timestamp
Review Comment:
[P2] Make this column TIMESTAMP_NTZ. This suite runs Spark 4.0 with the
default spark.sql.timestampType=TIMESTAMP_LTZ; Paimon maps that to
LocalZonedTimestampType, which this converter intentionally does not push. Both
assertions can therefore pass through Doris residual filtering even if the new
Timestamp.fromLocalDateTime precision fix is reverted. Please declare
event_time TIMESTAMP_NTZ and use a matching NTZ literal/cast, or set and
restore the Spark setting, so the regression exercises the changed NTZ pruning
path.
--
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]
