storage: enable sanity check for dynamic filter

[skyzh]

[fuyufjh]

@jon-chuang Please take a look

[jon-chuang]

We insert the RHS instead of updating. Just like hash agg (see: https://github.com/singularity-data/risingwave/issues/3885), we are guaranteed to do this only once per epoch.

Is it strictly necessary to update? If not, is it better to update instead of insert for any reason?

[skyzh]

Is it strictly necessary to update? If not, is it better to update instead of insert for any reason?

Currently it's not strictly required, but the StateTable interface was designed not to support using insert as update. I'm not sure whether this would cause problems in the future, and I think it would be okay to hold this issue for a while until we found issues with that.

From my perspective, HashAgg should have the old value stored in its LRU, so it won't cost more to provide the old value to StateTable 🤣

[jon-chuang]

Yes, the situation is the same for DynamicFilter, we can easily store the old value.

Well, I guess it does cost more to serialize the old row etc. But I'm definitely down with using update if it makes our usage of the interface more consistent.

[github-actions[bot]]

This issue has been open for 60 days with no activity. Could you please update the status? Feel free to continue discussion or close as not planned.

[BugenZhao]

Actually, the interface provided by the storage engine is actually with the "upsert" semantics... and the update interface, which does nothing except for the sanity checking, did not exist initially. 🤔

[stdrc]

What about we make that interface a real upsert, by merge part of #5191. Currently insert acts like upsert but only allow one insert with same pk within one epoch.