github-actions[bot] commented on code in PR #68848:
URL: https://github.com/apache/doris/pull/68848#discussion_r4236048855
##########
fe/fe-connector/fe-connector-hive/src/test/java/org/apache/doris/connector/hive/HiveScanBatchModeTest.java:
##########
@@ -462,6 +469,128 @@ public void missingReusePropertyDoesNotReuse() {
"an older planning FE's missing property must not enable reuse
in a newer connector");
}
+ @Test
+ public void statementReuseKeepsScansWithDifferentInputsApart() {
+ // Separately built but equal handles share one plan; a scan that
differs in any input the planned ranges
+ // depend on plans on its own (the ranges carry the partition files
and the BE file format).
+ // MUTATION: dropping a fact from HiveScanReuseKey (or the catalog id
from the memo key) makes that scan
+ // return an earlier plan -> red.
+ HiveScanPlanProvider provider = provider(new FakeHmsClient(), new
CountingLister());
+ TestStatementScope scope = new TestStatementScope();
+ ConnectorSession session = new ScopeSession(7L, "same-statement",
scope);
+ HiveTableHandle text = textHandle("t", LAZY_SIMPLE_SERDE,
part("year=2024/month=01"));
+
+ List<ConnectorScanRange> plain = provider.planScan(session,
scanOf(text));
+ Assertions.assertEquals(1, plain.size());
+ Assertions.assertSame(plain, provider.planScan(session,
+ scanOf(textHandle("t", LAZY_SIMPLE_SERDE,
part("year=2024/month=01")))),
+ "separately built but equal handles must share one plan");
+ List<ConnectorScanRange> otherPartition = provider.planScan(session,
Review Comment:
[P2] Exercise partition metadata independently in this reuse test. Every
partitioned `t` handle uses `PART_KEYS`, `part(name)` has empty HMS values, and
`otherPartition` changes location. Dropping key order from `HiveScanReuseKey`,
or values from `HmsPartitionInfo` equality/hash, would leave these assertions
green while reused ranges could carry the wrong BE partition columns. Add
nonempty-value pairs that vary key order and, separately, values at one
location; assert their range partition maps.
##########
fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergScanPlanProviderTest.java:
##########
@@ -629,6 +630,116 @@ public void
statementReuseStillValidatesMetadataColumnReader() {
ex.getMessage());
}
+ @Test
+ public void statementReuseKeepsScansWithDifferentPlanningInputsApart() {
+ // The memo caches the final range list, so every handle or request
fact those ranges depend on must be
+ // in the reuse key. COUNT pushdown, the ref and the rewrite scope
change the ranges themselves: sharing
+ // the plan would hand a COUNT's single collapsed range, the latest
snapshot's files, or the whole table
+ // to a scan that must read something else. MUTATION: dropping any
fact from IcebergScanReuseKey (or the
+ // catalog id from the memo key) makes that scan return an earlier
plan -> red.
+ Table table = createTable("t1", SCHEMA, PartitionSpec.unpartitioned());
+ table.newAppend().appendFile(dataFile(table.spec(),
"s3://b/db/t1/f1.parquet", 1000, null, null)).commit();
+ long s1 = table.currentSnapshot().snapshotId();
+ table.manageSnapshots().createTag("tag1", s1).commit();
+ table.newAppend().appendFile(dataFile(table.spec(),
"s3://b/db/t1/f2.parquet", 2000, null, null)).commit();
+ long s2 = table.currentSnapshot().snapshotId();
+ int schemaAtS2 = table.schema().schemaId();
+ table.updateSchema().addColumn("extra",
Types.IntegerType.get()).commit();
+ int laterSchema = table.schema().schemaId();
+ IcebergScanPlanProvider provider = providerOver(table);
+ TestStatementScope scope = new TestStatementScope();
+ ConnectorSession session = reuseSession(scope, 0L);
+ IcebergTableHandle latest = new IcebergTableHandle("db1", "t1");
+ IcebergTableHandle pinnedS2 = latest.withSnapshot(s2, null,
schemaAtS2);
+
+ List<ConnectorScanRange> plain = provider.planScan(session,
scanOf(latest).build());
+ Assertions.assertEquals(2, plain.size());
+ Assertions.assertSame(plain, provider.planScan(session,
scanOf(latest).build()));
+ List<ConnectorScanRange> atS2 = provider.planScan(session,
scanOf(pinnedS2).build());
+ Assertions.assertEquals(2, atS2.size());
+ List<ConnectorScanRange> atS1 = provider.planScan(session,
+ scanOf(latest.withSnapshot(s1, null, schemaAtS2)).build());
+ Assertions.assertEquals(1, atS1.size());
+ List<ConnectorScanRange> viaTag = provider.planScan(session,
+ scanOf(latest.withSnapshot(s2, "tag1", schemaAtS2)).build());
+ Assertions.assertEquals(1, viaTag.size(), "the tag reads only f1
although the handle names s2");
+ List<ConnectorScanRange> laterSchemaAtS2 = provider.planScan(session,
+ scanOf(latest.withSnapshot(s2, null, laterSchema)).build());
+ List<ConnectorScanRange> filtered = provider.planScan(session,
+ scanOf(latest).filter(Optional.of(equalIdFilter(1))).build());
+ List<ConnectorScanRange> otherFilter = provider.planScan(session,
+ scanOf(latest).filter(Optional.of(equalIdFilter(2))).build());
+ List<ConnectorScanRange> otherTable = provider.planScan(session,
+ scanOf(new IcebergTableHandle("db1", "t2")).build());
Review Comment:
[P2] Exercise the database component of the Iceberg reuse key. `otherTable`
changes only `t1` to `t2`, and `otherCatalog` changes the memo namespace; every
handle still uses `db1`. Removing `dbName` from `IcebergScanReuseKey` would
leave this test green while same-named tables in two databases could share
ranges. Add a second-database `t1` scan in the same statement and assert a
separate plan.
##########
fe/fe-connector/fe-connector-hive/src/test/java/org/apache/doris/connector/hive/HiveScanBatchModeTest.java:
##########
@@ -462,6 +469,128 @@ public void missingReusePropertyDoesNotReuse() {
"an older planning FE's missing property must not enable reuse
in a newer connector");
}
+ @Test
+ public void statementReuseKeepsScansWithDifferentInputsApart() {
+ // Separately built but equal handles share one plan; a scan that
differs in any input the planned ranges
+ // depend on plans on its own (the ranges carry the partition files
and the BE file format).
+ // MUTATION: dropping a fact from HiveScanReuseKey (or the catalog id
from the memo key) makes that scan
+ // return an earlier plan -> red.
+ HiveScanPlanProvider provider = provider(new FakeHmsClient(), new
CountingLister());
+ TestStatementScope scope = new TestStatementScope();
+ ConnectorSession session = new ScopeSession(7L, "same-statement",
scope);
+ HiveTableHandle text = textHandle("t", LAZY_SIMPLE_SERDE,
part("year=2024/month=01"));
+
+ List<ConnectorScanRange> plain = provider.planScan(session,
scanOf(text));
+ Assertions.assertEquals(1, plain.size());
+ Assertions.assertSame(plain, provider.planScan(session,
+ scanOf(textHandle("t", LAZY_SIMPLE_SERDE,
part("year=2024/month=01")))),
+ "separately built but equal handles must share one plan");
+ List<ConnectorScanRange> otherPartition = provider.planScan(session,
+ scanOf(textHandle("t", LAZY_SIMPLE_SERDE,
part("year=2024/month=02"))));
+ List<ConnectorScanRange> otherTable = provider.planScan(session,
Review Comment:
[P2] Cover same-catalog database identity too. `textHandle` hardcodes `db`;
`otherTable` changes only the table name and `otherCatalog` changes only the
memo namespace. Removing `dbName` from `HiveScanReuseKey` would leave this test
green, although `resolvePartitions` can read different HMS locations for
`db1.t` and `db2.t`. Use unpruned handles identical except `dbName` and a fake
HMS returning different paths by database; assert separate plans and paths.
##########
fe/fe-connector/fe-connector-paimon/src/test/java/org/apache/doris/connector/paimon/PaimonScanPlanProviderTest.java:
##########
@@ -906,6 +909,145 @@ public void
fileCreationTimeScanDoesNotApplyLimitToDiscardedTableScan(
}
}
+ @Test
+ public void
statementReuseKeepsScansWithDifferentPlanningInputsApart(@TempDir Path
warehouse) throws Exception {
+ // The memo caches the final range list (split, routed and
COUNT-collapsed), so every fact those ranges
+ // depend on must be in the reuse key. LIMIT and COUNT pushdown change
the ranges themselves: sharing the
+ // plan would hand LIMIT 1's single split or the COUNT range to a scan
that must read every row.
+ // MUTATION: dropping any fact from PaimonScanReuseKey (or the catalog
id from the memo key) makes that
+ // scan return an earlier plan -> red.
+ try (Catalog catalog = new FileSystemCatalog(LocalFileIO.create(),
+ new org.apache.paimon.fs.Path(warehouse.toUri()))) {
+ catalog.createDatabase("db", false);
+ Identifier id = Identifier.create("db", "reuse_key");
+ catalog.createTable(id, Schema.newBuilder()
+ .column("id", DataTypes.INT())
+ .column("pt", DataTypes.INT())
+ .partitionKeys("pt")
+ .option("bucket", "1")
+ .option("bucket-key", "id")
+ .build(), false);
+ Table table = catalog.getTable(id);
+ commitRows(table, GenericRow.of(1, 1), GenericRow.of(2, 2));
+ long s1 =
table.latestSnapshot().orElseThrow(AssertionError::new).id();
+ commitRows(table, GenericRow.of(3, 1));
+ long s2 =
table.latestSnapshot().orElseThrow(AssertionError::new).id();
+
+ RecordingPaimonCatalogOps ops = new RecordingPaimonCatalogOps();
+ ops.table = table;
+ ops.latestSnapshotId = OptionalLong.of(s2);
+ PaimonScanPlanProvider provider = new PaimonScanPlanProvider(
+ PaimonCatalogProperties.of(Collections.emptyMap()), ops);
+ TestStatementScope scope = new TestStatementScope();
+ ConnectorSession session = reuseSession(scope, 0L);
+ PaimonTableHandle latest = new PaimonTableHandle(
+ "db", "reuse_key", Collections.emptyList(),
Collections.emptyList());
+
+ List<ConnectorScanRange> plain = provider.planScan(session,
scanOf(latest).build());
+ Assertions.assertTrue(plain.size() >= 2, "fixture must plan one
split for each partition");
+ Assertions.assertSame(plain, provider.planScan(session,
scanOf(latest).build()));
+ List<ConnectorScanRange> atS1 = provider.planScan(session,
scanOf(pinnedTo(latest, s1)).build());
+ List<ConnectorScanRange> atS2 = provider.planScan(session,
scanOf(pinnedTo(latest, s2)).build());
+ Map<String, String> sinceS1 = new LinkedHashMap<>();
+ sinceS1.put("incremental-between", s1 + "," + s2);
+ sinceS1.put("incremental-between-scan-mode", "delta");
+ Map<String, String> sinceS1Reordered = new LinkedHashMap<>();
+ sinceS1Reordered.put("incremental-between-scan-mode", "delta");
+ sinceS1Reordered.put("incremental-between", s1 + "," + s2);
+ List<ConnectorScanRange> delta = provider.planScan(session,
+ scanOf(latest.withScanOptions(sinceS1)).build());
+ Assertions.assertSame(delta, provider.planScan(session,
+ scanOf(latest.withScanOptions(sinceS1Reordered)).build()),
+ "the same bounded incremental range given in another
option order must reuse");
+ List<ConnectorScanRange> filtered = provider.planScan(session,
+
scanOf(latest).filter(Optional.of(equalIdFilter(1))).build());
+ List<ConnectorScanRange> otherFilter = provider.planScan(session,
+
scanOf(latest).filter(Optional.of(equalIdFilter(2))).build());
+ List<ConnectorScanRange> projected = provider.planScan(session,
ConnectorScanRequest.builder(
+ latest, Collections.singletonList(new
PaimonColumnHandle("id", 0))).build());
+ List<ConnectorScanRange> limited = provider.planScan(session,
scanOf(latest).limit(1).build());
+ Assertions.assertEquals(1, limited.size(), "LIMIT 1 stops split
planning after one split");
+ List<ConnectorScanRange> count = provider.planScan(session,
scanOf(latest).countPushdown(true).build());
+ Assertions.assertEquals(1, count.size(), "COUNT pushdown collapses
the splits into one range");
+ Assertions.assertEquals("3",
count.get(0).getProperties().get("paimon.row_count"));
+ List<ConnectorScanRange> onBranch = provider.planScan(session,
+ scanOf(latest.withBranch("audit")).build());
+ List<ConnectorScanRange> otherCatalog =
provider.planScan(reuseSession(scope, 1L),
Review Comment:
[P2] Add same-catalog relation-name variants. All normal scans here use
`db.reuse_key`; `otherCatalog` changes only the outer memo key. Removing
`databaseName` or `tableName` from `PaimonScanReuseKey` would leave this test
green even though a second relation could get the first table's splits. Scan
another table and a same-named table in another database within this scope,
keeping the generation equal so only each name field separates its key, and
assert separate plans.
--
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]
