github-actions[bot] commented on code in PR #67891:
URL: https://github.com/apache/doris/pull/67891#discussion_r4046123740
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/ForeignKeyContext.java:
##########
@@ -131,32 +130,81 @@ void putAllPrimaryKeys(TableIf table) {
for (PrimaryKeyConstraint c :
Env.getCurrentEnv().getConstraintManager()
.getPrimaryKeyConstraints(tableNameInfo)) {
Set<QualifiedColumn> primaryKey = c.getPrimaryKeys(table).stream()
- .map(column -> new QualifiedColumn(table,
column)).collect(Collectors.toSet());
- primaryKeys.addAll(primaryKey);
+ .map(column -> new QualifiedColumn(table, column))
+ .collect(ImmutableSet.toImmutableSet());
+ declaredPrimaryKeys.add(primaryKey);
}
}
public boolean isForeignKey(Set<Slot> key) {
- return foreignKeys.containsAll(
- key.stream().map(s ->
slotToColumn.get(s)).collect(Collectors.toSet()));
+ Set<QualifiedColumn> columns = key.stream()
+ .map(slotToColumn::get)
Review Comment:
[P1] Preserve relation-instance identity for composite FKs
This exact column-set check can still assemble one composite FK from
different aliases of the same table. For example:
```text
Project(f1.fa, f2.fb)
Join(p.a = f1.fa AND p.b = f2.fb)
Scan composite_pri p
CrossJoin(Scan composite_foreign f1, Scan composite_foreign f2)
```
`slotToColumn` maps both aliases only to table+column identity, so `{f1.fa,
f2.fb}` becomes the declared `{fa, fb}` FK and `satisfyConstraint` accepts it
even though no single foreign row supplied that tuple. With PK/FK rows
`(1,1),(2,2)`, the real top join rejects mixed pairs `(1,2)` and `(2,1)`, while
eliminating `p` returns all four cross-product rows. This is distinct from the
earlier partial-composite-key thread: the complete key is present here, but it
comes from two relation instances. Please propagate a relation-instance/proof
token through aliases and require every column of one FK proof to share it,
with a cross-alias result regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/ForeignKeyContext.java:
##########
@@ -131,32 +130,81 @@ void putAllPrimaryKeys(TableIf table) {
for (PrimaryKeyConstraint c :
Env.getCurrentEnv().getConstraintManager()
.getPrimaryKeyConstraints(tableNameInfo)) {
Set<QualifiedColumn> primaryKey = c.getPrimaryKeys(table).stream()
- .map(column -> new QualifiedColumn(table,
column)).collect(Collectors.toSet());
- primaryKeys.addAll(primaryKey);
+ .map(column -> new QualifiedColumn(table, column))
+ .collect(ImmutableSet.toImmutableSet());
+ declaredPrimaryKeys.add(primaryKey);
}
}
public boolean isForeignKey(Set<Slot> key) {
- return foreignKeys.containsAll(
- key.stream().map(s ->
slotToColumn.get(s)).collect(Collectors.toSet()));
+ Set<QualifiedColumn> columns = key.stream()
+ .map(slotToColumn::get)
+ .collect(Collectors.toSet());
+ return !key.isEmpty() && !columns.contains(null)
+ && constraints.stream().anyMatch(constraint ->
constraint.keySet().equals(columns));
}
public boolean isPrimaryKey(Set<Slot> key) {
- return primaryKeys.containsAll(
- key.stream().map(s ->
slotToColumn.get(s)).collect(Collectors.toSet()));
+ return !key.isEmpty() && activePrimaryKeys.contains(key);
}
- void putSlot(SlotReference slot, TableIf table) {
- if (!slot.getOriginalColumn().isPresent()) {
- return;
+ void putSlots(LogicalCatalogRelation relation, TableIf table) {
+ Map<QualifiedColumn, Slot> columnToSlot = new HashMap<>();
+ for (Slot slot : relation.getOutput()) {
+ if (!(slot instanceof SlotReference) || !((SlotReference)
slot).getOriginalColumn().isPresent()) {
+ continue;
+ }
+ Column column = ((SlotReference) slot).getOriginalColumn().get();
+ QualifiedColumn qualifiedColumn = new QualifiedColumn(table,
column);
+ slotToColumn.put(slot, qualifiedColumn);
+ columnToSlot.put(qualifiedColumn, slot);
+ }
+
+ for (Set<QualifiedColumn> declaredPrimaryKey : declaredPrimaryKeys) {
+ if (!columnToSlot.keySet().containsAll(declaredPrimaryKey)) {
+ continue;
+ }
+ Set<Slot> primaryKey = declaredPrimaryKey.stream()
+ .map(columnToSlot::get)
+ .collect(ImmutableSet.toImmutableSet());
+ if (canActivatePrimaryKey(relation, primaryKey)) {
+ activePrimaryKeys.add(primaryKey);
+ }
}
- Column c = slot.getOriginalColumn().get();
- slotToColumn.put(slot, new QualifiedColumn(table, c));
+ }
+
+ private boolean canActivatePrimaryKey(LogicalCatalogRelation relation,
Set<Slot> primaryKey) {
+ if (!relation.getLogicalProperties().getTrait().isUnique(primaryKey)) {
+ return false;
+ }
+ if (!(relation instanceof LogicalOlapScan)) {
+ return true;
Review Comment:
[P1] Require full-relation proof for external scans too
This branch treats every non-OLAP catalog relation as complete once its
declared key is unique, but `LogicalFileScan` can carry `TABLESAMPLE`, a
snapshot, or relation-scoped scan parameters. External PK/FK constraints are
supported, `BindRelation` forwards those selectors, and supported connectors
physically apply sampling. Thus:
```text
Project(f.parent_id)
Join(p.id = f.parent_id)
LogicalFileScan parent TABLESAMPLE(1 ROWS) p
LogicalFileScan child f
```
can activate `p.id` from the catalog constraint even when the sample
contains only key `1`; eliminating `p` then returns child key `2`, which the
real join rejects. This is distinct from the existing OLAP thread because the
new early return bypasses all of its `LogicalOlapScan` guards. Please require a
scan-specific full-relation proof for every `LogicalCatalogRelation` (at
minimum reject external sample/snapshot/scan-param states unless proven
complete) and add an external-selector regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/ForeignKeyContext.java:
##########
@@ -131,32 +130,81 @@ void putAllPrimaryKeys(TableIf table) {
for (PrimaryKeyConstraint c :
Env.getCurrentEnv().getConstraintManager()
.getPrimaryKeyConstraints(tableNameInfo)) {
Set<QualifiedColumn> primaryKey = c.getPrimaryKeys(table).stream()
- .map(column -> new QualifiedColumn(table,
column)).collect(Collectors.toSet());
- primaryKeys.addAll(primaryKey);
+ .map(column -> new QualifiedColumn(table, column))
+ .collect(ImmutableSet.toImmutableSet());
+ declaredPrimaryKeys.add(primaryKey);
}
}
public boolean isForeignKey(Set<Slot> key) {
- return foreignKeys.containsAll(
- key.stream().map(s ->
slotToColumn.get(s)).collect(Collectors.toSet()));
+ Set<QualifiedColumn> columns = key.stream()
+ .map(slotToColumn::get)
+ .collect(Collectors.toSet());
+ return !key.isEmpty() && !columns.contains(null)
+ && constraints.stream().anyMatch(constraint ->
constraint.keySet().equals(columns));
}
public boolean isPrimaryKey(Set<Slot> key) {
- return primaryKeys.containsAll(
- key.stream().map(s ->
slotToColumn.get(s)).collect(Collectors.toSet()));
+ return !key.isEmpty() && activePrimaryKeys.contains(key);
}
- void putSlot(SlotReference slot, TableIf table) {
- if (!slot.getOriginalColumn().isPresent()) {
- return;
+ void putSlots(LogicalCatalogRelation relation, TableIf table) {
+ Map<QualifiedColumn, Slot> columnToSlot = new HashMap<>();
+ for (Slot slot : relation.getOutput()) {
+ if (!(slot instanceof SlotReference) || !((SlotReference)
slot).getOriginalColumn().isPresent()) {
+ continue;
+ }
+ Column column = ((SlotReference) slot).getOriginalColumn().get();
+ QualifiedColumn qualifiedColumn = new QualifiedColumn(table,
column);
+ slotToColumn.put(slot, qualifiedColumn);
+ columnToSlot.put(qualifiedColumn, slot);
+ }
+
+ for (Set<QualifiedColumn> declaredPrimaryKey : declaredPrimaryKeys) {
+ if (!columnToSlot.keySet().containsAll(declaredPrimaryKey)) {
+ continue;
+ }
+ Set<Slot> primaryKey = declaredPrimaryKey.stream()
+ .map(columnToSlot::get)
+ .collect(ImmutableSet.toImmutableSet());
+ if (canActivatePrimaryKey(relation, primaryKey)) {
+ activePrimaryKeys.add(primaryKey);
+ }
}
- Column c = slot.getOriginalColumn().get();
- slotToColumn.put(slot, new QualifiedColumn(table, c));
+ }
+
+ private boolean canActivatePrimaryKey(LogicalCatalogRelation relation,
Set<Slot> primaryKey) {
+ if (!relation.getLogicalProperties().getTrait().isUnique(primaryKey)) {
+ return false;
+ }
+ if (!(relation instanceof LogicalOlapScan)) {
+ return true;
+ }
+ LogicalOlapScan scan = (LogicalOlapScan) relation;
+ return new HashSet<>(scan.getSelectedPartitionIds()).equals(
+ new HashSet<>(scan.getTable().getPartitionIds()))
+ && scan.getSelectedTabletIds().isEmpty()
+ && !scan.getTableSample().isPresent()
+ && !scan.isDirectMvScan();
}
void putAlias(Slot newSlot, Slot originSlot) {
if (slotToColumn.containsKey(originSlot)) {
slotToColumn.put(newSlot, slotToColumn.get(originSlot));
+ Set<Set<Slot>> aliasedPrimaryKeys = activePrimaryKeys.stream()
+ .filter(primaryKey -> primaryKey.contains(originSlot))
Review Comment:
[P1] Avoid power-set growth when aliasing composite keys
For a declared key `(a1, ..., aN)` under `Project(a1 AS x1, ..., aN AS xN)`,
this loop keeps every old key and adds every version with the current component
replaced. The number of `activePrimaryKeys` therefore doubles for each alias:
`{a,b,c}` becomes 2 sets after `a AS x`, 4 after `b AS y`, and 8 after `c AS
z`, even though only `{x,y,z}` is visible above the project. In general this
allocates `2^N` sets and copies `Theta(N * 2^N)` slot references; 20 columns
already create 1,048,576 sets during planning. Please keep alias lineage in a
compact per-component representation (or batch the project and retain only
output-reachable proofs) rather than enumerating the Cartesian product, and add
a many-column alias test that bounds proof growth.
--
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]
