yujun777 commented on code in PR #68646:
URL: https://github.com/apache/doris/pull/68646#discussion_r4140859683
##########
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;
Review Comment:
Fixed in c3b24e67ee1. The per-output copy is gone; the check scans
`getDatasetIndirectLineageMap().values()` once. The copy the lineage builds for
each output column holds the same expressions for every column the query
produces, so it added nothing to this predicate, and the expressions it does
not carry -- the WINDOW and CONDITIONAL ones -- are reached through the direct
map's own input slots.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java:
##########
@@ -373,8 +374,30 @@ private boolean
processAlterOlapTableInternal(List<AlterOp> alterOps, OlapTable
throw new DdlException("Invalid alter operations: " +
currentAlterOps);
}
if (needChangeMTMVState(alterOps)) {
- Env.getCurrentEnv().getMtmvService()
- .alterTable(oldBaseTableInfo, newBaseTableInfo,
currentAlterOps.hasReplaceTableOp());
+ // Which columns an operation's effect on a view turns on is the
operation's to say, see
+ // AlterOp#queryJudgedColumnNames, and every clause of the alter
has to name them: a batch that
+ // mixes a dropped column with a type change is decided by neither
-- no query says anything
+ // about a type change -- and stays invalidated the way it was
before the queries were asked at
+ // all. Each of them also has to have reached the table. A schema
change that is not a light one
+ // is applied by a job, which may not have run where this hook
runs: the table still holds the
+ // column the change takes away, every query still analyses
against it, and an invalidation
+ // decided on that answer would be about the table from before the
change. What is asked is
+ // whether the change has reached the table, which is the same
fact the re-analysis reads, so
+ // the two answers cannot disagree.
+ boolean judgedByQuery = alterOps.stream().allMatch(op ->
!op.queryJudgedColumnNames().isEmpty()
Review Comment:
Fixed in c3b24e67ee1. `AddColumnOp` and `AddColumnsOp` no longer take the
default answer: they ask whether the table now holds every column the add is
about, so an add a job has not applied yet is not judged by a query analysed
against the schema from before it, and the MV is invalidated the way it was
before the queries were asked at all.
Why the question is asked per column rather than about the table's shape is
your case: `SELECT 1 AS flag, COUNT(*) AS n FROM t GROUP BY flag HAVING flag =
1` goes on analysing while `flag` moves off the alias onto the new base column,
so what changed is which column answers for the name, and the check decides it
on that.
Worth recording that my first attempt here was wrong: asking whether the
columns are in the table instead of asking the change re-invalidated every add
of a column no query reaches. The suite catches it -- an add of a column the
query cannot reach must leave the view where it is.
--
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]
