Fix(risingwave): Recreate materialized views#5195
Conversation
|
Thanks @VaggelisD for looking into this! I tested your fix and the main reported issue is resolved - the plan command no longer fails. However, I wonder if we could avoid dropping and then recreating exactly the same MV for RisingWave. It seems to be a regression compared to 0.197.4. Why do we want to do it in the streaming context? Log PR51952025-08-21 08:48:45,328 - MainThread - sqlmesh.core.plan.evaluator - INFO - Evaluating plan stage CreateSnapshotRecordsStage (evaluator.py:125) In contrast, in 0.197.4, a MV is only created once. Log 0.197.42025-08-21 08:59:47,841 - MainThread - sqlmesh.core.plan.evaluator - INFO - Evaluating plan stage CreateSnapshotRecordsStage (evaluator.py:115) |
|
Hey @xardasos, thank you for confirming. Check out the explanation in the initial PR, the solution here was intended to cover all the other dialects. However, I tested the problematic scenario (i.e dropping the underlying table, which invalidates the mview in other engines) and that doesn't seem to be allowed in RisingWave, e.g: dev=> create table bar as (select 1 as col);
INSERT 0 1
dev=> create materialized view foo as (select * from bar);
CREATE_MATERIALIZED_VIEW
dev=> drop table bar;
ERROR: Failed to run the query. Caused by: Permission denied: table used by 1 other objects.
dev=> CREATE OR REPLACE TABLE bar AS (SELECT 2 AS col);
ERROR: Failed to run the query Caused by: Feature is not yet implemented: CREATE OR REPLACE TABLEThus, the reason RisingWave never had an issue with materialized views is because even for I can push a new fix to not recreate the views in RW and you can give it a shot if you want. |
VaggelisD
force-pushed
the
vaggelisd/mview_replace
branch
from
30882da to
636c1db
Compare
|
Thanks @VaggelisD for explaining. The newest version seems to be working correctly, nice! |
VaggelisD
force-pushed
the
vaggelisd/mview_replace
branch
3 times, most recently
from
f56d1fd to
b0ea051
Compare
VaggelisD
changed the title
|
@VaggelisD I discovered one more problem in the current version. In case of indirect non-breaking changes, the MVs are not being recreated when it might be necessary. Let's assume we have : In 0.197.4 sqlmesh also drops B_567 and recreates it so that it now consumes from A_124 instead of A_123. In this new version this recreation doesn't happen. This can cause issues when the previous version A_123 gets finally cascade deleted (B_567 still consumes from A_123). |
|
@xardasos This is expected afaict, the indirect non-breaking changes don't trigger a backfill. Do you mean that In any event, if the optimization here is flaky I think it'd be best if I revert it and instead force the RisingWave adapter to always recreate the views. |
|
@VaggelisD, if we don't recreate B, then MV B_567 continues to be physically attached to A_123 (B_567 is defined in the DB as select id from A_123 - RW MVs are tightly coupled) while sqlmesh virtual view A points to the new MV A_124. Eventually, we want to get rid of the old MV A_123 in favor of A_124 (I guess we don't want to keep both versions A_123 and A_124 forever). However, if we do this, then B_567 and sqlmesh virtual view B get deleted as well due to the tight coupling and the drop cascading effect which probably is not what we want. I think we should keep the optimization in the most basic and common workflow (adding a new MV) and at least not make things worse compared to 0.197.4. Having to create each MV twice (when it is not necessary), in my opinion, is a huge performance blow, as it sometimes takes hours or even days to create a big MV. However, after investigating this a bit more I think that your changes are now correct. I'm not 100% sure, but I now suspect that the reason why the recreation no longer happens could be the very recent changes from #5189. It looks to me that one problem with these changes is that now (I'll be away until 07.09 and may not be able to reply during this time.) |
|
Hey @xardasos, with that in mind I think I'll reenable the force recreation since the MV will either (1) be replaced due to a breaking/indirect breaking change or (2) it should be replaced in indirect non breaking to avoid stale references, meaning that it should probably always be replaced. If you can optimize this further it'd be great if you can contribute a well-tested PR with your additional usecases. |
VaggelisD
changed the title
VaggelisD
force-pushed
the
vaggelisd/mview_replace
branch
from
4424859 to
147d85d
Compare
VaggelisD
force-pushed
the
vaggelisd/mview_replace
branch
from
147d85d to
ff2e7f5
Compare
VaggelisD
force-pushed
the
vaggelisd/mview_replace
branch
from
ff2e7f5 to
c42ced4
Compare
|
Hey @VaggelisD, thanks again for this fix, it is definitely a step in the right direction. However, I think there is still a regression compared to the version 0.197.4, where MVs are created only once in basic scenarios (e.g. adding a new MV) and the indirect non breaking changes are handled correctly. I have created a follow-up issue #5331. |
5 participants
Fixes #5184