github-actions[bot] commented on code in PR #66007:
URL: https://github.com/apache/doris/pull/66007#discussion_r3651779242
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalFileScan.java:
##########
@@ -65,6 +68,8 @@ public class LogicalFileScan extends LogicalCatalogRelation
implements SupportPr
protected final Optional<TableSnapshot> tableSnapshot;
protected final Optional<TableScanParams> scanParams;
protected final Optional<List<Slot>> cachedOutputs;
+ protected final Optional<List<Column>> relationSchema;
+ protected final Optional<MvccSnapshot> relationSnapshot;
Review Comment:
[P1] Include the resolved snapshot in scan equality
This new state is preserved by the generic copy paths, but
`hasSameScanState` still compares only selected partitions, sample, and the raw
snapshot/ref parameters. If branch `b` advances between two bindings,
`t@branch(b)` can carry S0 and S1 while still comparing equal;
`PullUpJoinFromUnionAll` may then hoist one scan from `Union(Join(A,t@b[S0]),
Join(B,t@b[S1]))` and replace the second arm's data. Please either fence
identical refs to one statement snapshot or include a stable resolved snapshot
identity in semantic equality, with a rewrite test for distinct resolved heads.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HMSExternalTable.java:
##########
@@ -392,6 +392,19 @@ public List<Column> getFullSchema() {
return schemaCacheValue.map(SchemaCacheValue::getSchema).orElse(null);
}
+ @Override
+ public List<Column> getFullSchema(Optional<MvccSnapshot> snapshot) {
+ makeSureInitialized();
+ // HMS can front snapshot-aware formats too; callers that pin a
relation must not fall
+ // back to the table's current schema merely because its catalog type
is HMS.
+ if (getDlaType() == DLAType.HUDI) {
Review Comment:
[P1] Carry the snapshot through the specialized Hudi scan
This overload is snapshot-aware, but the Hudi arm of `BindRelation` still
creates `LogicalHudiScan` through its legacy path. That leaves its relation
schema uncaptured, its logical/physical copies do not preserve the resolved
snapshot, and `visitPhysicalHudiScan` never installs it on `HudiScanNode`;
those paths fall back to the last table-scoped snapshot. A query mixing latest
and historical Hudi relations across a column or partition-schema evolution can
therefore bind or scan one side with the other side's metadata. Please carry
both relation schema and snapshot through the Hudi-specific logical, physical,
and translator path and cover both relation orders.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalFileScan.java:
##########
@@ -73,12 +78,24 @@ public LogicalFileScan(RelationId id, ExternalTable table,
List<String> qualifie
Collection<Slot> operativeSlots,
Optional<TableSample> tableSample, Optional<TableSnapshot>
tableSnapshot,
Optional<TableScanParams> scanParams, Optional<List<Slot>>
cachedOutputs) {
+ this(id, table, qualifier, operativeSlots, tableSample, tableSnapshot,
scanParams, cachedOutputs,
+ MvccUtil.getSnapshotFromContext(table));
+ }
+
+ /**
+ * Constructor for a relation whose concrete snapshot was resolved during
binding.
+ */
+ public LogicalFileScan(RelationId id, ExternalTable table, List<String>
qualifier,
+ Collection<Slot> operativeSlots,
+ Optional<TableSample> tableSample, Optional<TableSnapshot>
tableSnapshot,
+ Optional<TableScanParams> scanParams, Optional<List<Slot>>
cachedOutputs,
+ Optional<MvccSnapshot> relationSnapshot) {
this(id, table, qualifier,
-
table.initSelectedPartitions(MvccUtil.getSnapshotFromContext(table)),
+ table.initSelectedPartitions(relationSnapshot),
Review Comment:
[P1] Use the relation snapshot for partition pruning
The partition map is now frozen from `relationSnapshot` here, but
`PruneFileScanPartition` still fetches both partition-column lists from
`StatementContext.getSnapshot(externalTable)`. After another version/ref of the
same table is bound, that table-scoped entry can be S1 while this scan and its
partition map are S0. With partition-schema/spec evolution, the rule then maps
S1 columns into S0 output and can prune with null or wrong slots. Please use
`scan.getRelationSnapshot()` for all pruning metadata and add a filtered
dual-version case.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/StatementContext.java:
##########
@@ -982,15 +983,23 @@ public void addPlannerHook(PlannerHook plannerHook) {
* @param tableSnapshot table snapshot info
* @param scanParams table scan params (e.g., branch/tag for Iceberg
tables)
*/
- public void loadSnapshots(TableIf specificTable, Optional<TableSnapshot>
tableSnapshot,
+ public Optional<MvccSnapshot> loadSnapshots(TableIf specificTable,
Optional<TableSnapshot> tableSnapshot,
Optional<TableScanParams> scanParams) {
- if (specificTable instanceof MvccTable) {
- MvccTableInfo mvccTableInfo = new MvccTableInfo(specificTable);
- if (!snapshots.containsKey(mvccTableInfo)) {
- snapshots.put(mvccTableInfo,
- ((MvccTable)
specificTable).loadSnapshot(tableSnapshot, scanParams));
- }
+ if (!(specificTable instanceof MvccTable)) {
+ return Optional.empty();
+ }
+ MvccTableInfo mvccTableInfo = new MvccTableInfo(specificTable);
+ MvccSnapshot snapshot;
+ if (tableSnapshot.isPresent() || scanParams.isPresent()) {
+ snapshot = ((MvccTable) specificTable).loadSnapshot(tableSnapshot,
scanParams);
+ } else {
+ // Keep latest metadata separate: a historical relation may
temporarily become the
+ // table-scoped snapshot, but it must not redefine what a later
latest relation sees.
+ snapshot = latestSnapshots.computeIfAbsent(mvccTableInfo,
Review Comment:
[P1] Preserve the snapshot injected by MTMV refresh
`MTMVTask.beforeMTMVRefresh` captures each MVCC base-table snapshot, and
every `exec` injects those exact values into a new `StatementContext` with
`setSnapshot`. Because that setter fills only `snapshots`, this new cache is
empty: the first unqualified binding reloads S1 here and overwrites the
injected S0. This discards the task's captured execution fence, so later
refresh batches or retries can bind a newer metadata generation than the one
the task deliberately captured. Please make the injected latest snapshot
authoritative for this cache and add an injection-then-unqualified-load test.
##########
regression-test/suites/external_table_p0/iceberg/test_iceberg_schema_dual_relation_matrix.groovy:
##########
@@ -93,10 +93,24 @@ suite("test_iceberg_schema_dual_relation_matrix",
order by id
"""))
- // Scenario TC07-join negative contract:
- // two historical relations in one statement currently reuse the first
schema.
- test {
- sql """
+ // Latest metadata must be restored even when a historical relation is
bound first.
+ assertEquals([[1, "old-1", "old-1"]], sql("""
Review Comment:
[P2] Record deterministic results in generated oracles
These new matrix queries have exact, ordered expected rows, but they are
asserted only in Groovy and none of the matrix suites has a generated `.out`
artifact. That bypasses the repository's required `qt_`/result-generation
workflow for determined outputs and makes the contract invisible to normal
golden-result review. Please convert the deterministic positive-result checks
to ordered `qt_` cases and generate the matching `.out` files; keep direct
assertions only for genuinely dynamic metadata predicates.
--
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]
