refactor(state table): remove commit_no_data_expected

[stdrc]

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

To me, the StateTable::commit_no_data_expected introduced unnecessary mental burden. Developers have to somehow track whether the state is dirty or not in executors, while this information is clearly known by state table itself.

This PR removes the unnecessary method, and uniformly uses commit instead. Inside commit, the state table will branch according to StateTable::is_dirty so that the calling cost is not changed.

I'm doing benchmark to test the performance impact.

Checklist

• I have written necessary rustdoc comments
• I have added necessary unit tests and integration tests
• I have added test labels as necessary. See details.
• I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
• My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
• All checks passed in ./risedev check (or alias, ./risedev c)
• My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

• My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

• My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

[wenym1]

LGTM!

[kwannoel]

LGTM once tests pass. Better to incur the branch in StateTable::commit than in executor IMO.

[BugenZhao]

I believe it was introduced as an optional assertion for developers. According to the changes, however, some developers are taking pains to call it even if not necessary. So +1 for removing that.

[stdrc]

I believe it was introduced as an optional assertion for developers.

Yes. However things get complicated after we introducing state cleaning. Even without data changed, updating state cleaning watermark will also require commit instead of commit_no_data_expected, which forces executors to take too much care of the internal details of state table.

[st1page]

LGTM, the method is used for LookUpJoin and used for the "read-only state table". I think it's original usage is a table-level option. But now, all places needing this have changed to use the BatchTable.

[stdrc]

Did some nexmark bench on q0 in normal mode, q4~q9,q18 in eowc mode, results show almost no difference.