yujun777 commented on code in PR #68646:
URL: https://github.com/apache/doris/pull/68646#discussion_r4140858555
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/RenameColumnOp.java:
##########
@@ -97,6 +100,19 @@ public boolean needChangeMTMVState() {
return true;
}
+ @Override
Review Comment:
Fixed in c3b24e67ee1, and pinned in b31abff23e6. `RenameColumnOp` reports
both names now -- the one the query spelled and the one that answers for it
from then on -- because neither of the two says on its own whether the query
moved: the old name is one no query reaches any more, and the new one is a move
only where the old one was the query's before.
On the shape in your comment the capture is then decided on `flag`: the
re-analysed query reaches `flag` from the table the rename was applied to,
which is the criterion the check now uses, so the MV is invalidated. The
suite's scope group covers the three ways a name moves into or out of a
subquery's scope -- an add, a drop, and a rename that hands a name to it --
each with its own golden, and each failing with its half of the change removed.
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVRelationManager.java:
##########
@@ -385,28 +396,74 @@ public void alterTable(BaseTableInfo oldTableInfo,
Optional<BaseTableInfo> newTa
* rows computed under the old column epoch. Invalidating the MV is what
keeps that from being
* reported as current.
*
+ * <p>The check is the criterion, not just the reason for the record: a
column the query does not name
+ * is one this change leaves the MV's rows alone for, so nothing is
invalidated for it. It is a whole
+ * query that is analysed, not a column that is looked up: what the MV can
no longer be computed from
+ * is what the analysis refuses, wherever in the query it stood.
+ *
* <p>Every MV is checked, not only an IVM one: whether the query still
analyzes is a property of
* the MV and of the base table it reads, not of how the MV refreshes, and
the invalidation is the
* same one a change to that table records. What an IVM MV has on top of
it is a per-partition
* requirement, and that is decided elsewhere, from a query that analyzed.
*
- * @return whether the MV was invalidated. That is the whole record for
this change: the invalidation
- * carries the reason, and the caller has nothing left to write --
a second record would land
- * on the same state, and MTMVStatus#updateStateAndDetail would
overwrite the detail with the
- * blunter "the base table has been updated", which is what
knowing the query is unusable is
- * for. It would also bump the version and drop the snapshot twice
for one change.
+ * @return whether the MV was invalidated. False is the answer for a
change that reaches neither the
+ * query nor the rows it computed, and it is the whole record for
that change: there is nothing to
+ * write, and writing the generic "the base table has been
updated" anyway would stand for a
+ * rebuild the MV does not owe.
+ */
+ /**
+ * Whether the query, as it is analysed now, reads a column of any of
these names.
+ *
+ * <p>The names are matched rather than the columns, and matched
case-insensitively, because a name is
+ * what the change moves: the column that goes away leaves its name to
whatever else answers to it, and
+ * the query that reaches the name afterwards is reading a column this
view's rows were not built from.
*/
- private boolean invalidateMvIfQueryUnusable(BaseTableInfo baseTableInfo,
Table mvTable) {
+ private static boolean reachesAnyColumnOf(Plan plan, Set<String>
columnNames) {
+ if (plan == null) {
+ // A query whose plan was not kept is one this cannot be answered
about, and "it does" is the
+ // answer that keeps the view safe.
+ return true;
+ }
+ Set<String> names = Sets.newTreeSet(String.CASE_INSENSITIVE_ORDER);
+ names.addAll(columnNames);
+ LineageInfo lineage = LineageInfoExtractor.extractLineageInfo(plan);
+ for (SetMultimap<?, Expression> byType :
lineage.getDirectLineageMap().values()) {
+ if (reachesAnyColumn(byType.values(), names)) {
+ return true;
+ }
+ }
+ for (SetMultimap<?, Expression> byType :
lineage.getInDirectLineageMapByDataset().values()) {
+ if (reachesAnyColumn(byType.values(), names)) {
+ return true;
+ }
+ }
+ return
reachesAnyColumn(lineage.getDatasetIndirectLineageMap().values(), names);
+ }
+
+ private static boolean reachesAnyColumn(Collection<Expression>
expressions, Set<String> names) {
Review Comment:
Fixed in c3b24e67ee1. The match is two-sided now, which is what your two
halves ask for.
A reached slot is a hit only when the column it is comes from the table the
change is about. That identity is read from the slot's own original table and
column, which fall through whatever views stand between the two, and it is
compared by name/catalog rather than by object, so a view is not a way around
it and a same-named column of another table is not a hit.
The name-level check is kept only for names the query resolves across a
scope boundary -- which is what the plan records as the correlation slots of
its Apply, the outer columns a subquery's own scope does not answer for. Those
are names the change can move and only those, which is why a drop and an add of
`a.flag` now leave `SELECT a.id FROM a JOIN b ON a.id = b.id WHERE b.flag = 1`
where it is (the suite pins both), while the inner-table rebinding case is
still invalidated.
One thing this does not separate, worth saying rather than leaving implied:
a correlated reference written with the outer qualifier (`o.flag` inside the
subquery) is in the correlation slots too, so a change to another table's
`flag` invalidates a view whose reference could not have moved. Telling those
apart needs the reference's own qualification, which the analysed plan has
resolved away -- the same missing half as the creation-time question in the
earlier thread.
--
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]
