github-actions[bot] commented on code in PR #68646:
URL: https://github.com/apache/doris/pull/68646#discussion_r4140420539
##########
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:
[P2] Keep the changed table's identity in this match. A refreshed MV `SELECT
a.id FROM a JOIN b ON a.id=b.id WHERE b.flag=1` does not use `a.flag`; adding
`a.flag` leaves the explicitly qualified `b.flag` binding and all MV rows
unchanged. The analyzed lineage still contains `b.flag`, but comparing only
`slot.getName()` with the changed name `flag` invalidates the MV, drops its
rewrite snapshot, and forces a whole refresh. The same occurs when an unused
`a.flag` is dropped or renamed. Match the bound source column, while retaining
a separate check for names that can actually rebind across scopes.
##########
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:
[P2] Avoid copying dataset predicates per output column here.
`getInDirectLineageMapByDataset()` builds a new multimap containing every
dataset filter/join/group/sort expression for each MV output slot, then line
440 scans the original dataset map again. On a wide MV with 1,000 outputs and
100 predicates, one qualifying ALTER allocates roughly 100,000 duplicate
entries per dependent MV before checking a name. Scan
`getDatasetIndirectLineageMap().values()` once; the per-output copies add no
information to this predicate.
##########
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:
[P1] Check the new name for a rename as well. An MV of `SELECT o.id FROM
outer_t o WHERE EXISTS (SELECT 1 FROM inner_t i WHERE i.id=o.id AND flag=1)`
can initially bind unqualified `flag` to `o.flag` when `inner_t` has `x` but no
`flag`. `RENAME COLUMN x flag` makes the same query bind `i.flag`, changing its
rows without changing its output schema. This method reports only `x`;
re-analyzed lineage contains `flag`, and `hasReachedTheTable` is true because
`x` is gone, so the MV stays NORMAL and eligible for rewrite with stale rows.
Include the new name (or compare bound column identities) before preserving the
MV. This is the rename capture case, distinct from the existing DROP thread.
##########
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:
[P1] Do not query-judge a deferred ADD before its new schema is installed.
On a table `t(id)` with `light_schema_change=false`, a refreshed MV of `SELECT
1 AS flag, COUNT(*) AS n FROM t GROUP BY flag HAVING flag=1` has row `(1,N)`:
both `flag` references bind the SELECT alias. `ADD COLUMN flag INT DEFAULT 0`
submits a schema job, but `AddColumnOp` inherits `hasReachedTheTable=true`, so
this branch re-analyzes the old schema, finds no base `flag` use, and leaves
the MV NORMAL. When the job finishes, GROUP BY/HAVING bind `t.flag` (0 for old
rows), so the query returns no row with the same output schema; no later MV
hook runs. Invalidate at submission or re-evaluate after job completion.
--
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]
