Skip to content

Threshold should only produce positive updates#2466

Merged
frankmcsherry merged 2 commits into
MaterializeInc:masterfrom
frankmcsherry:fix_2464
Mar 28, 2020
Merged

Threshold should only produce positive updates#2466
frankmcsherry merged 2 commits into
MaterializeInc:masterfrom
frankmcsherry:fix_2464

Conversation

@frankmcsherry

@frankmcsherry frankmcsherry commented Mar 28, 2020

Copy link
Copy Markdown
Contributor

Threshold would set negative diffs to zero, but that confused other parts of the system. Specifically (for future ref) differential's consolidate method, which we used, doesn't seem to expect zero diffs and does not proactively filter out those that it does not create.

Fixes MaterializeInc/database-issues#866

@benesch

benesch commented Mar 28, 2020

Copy link
Copy Markdown
Contributor

Hang on, this is a differential bug by my reckoning.

@benesch

benesch commented Mar 28, 2020

Copy link
Copy Markdown
Contributor

I have an alternative fix coming in shortly and then we can discuss!

@frankmcsherry

Copy link
Copy Markdown
Contributor Author

Hang on, this is a differential bug by my reckoning.

You'll have to convince me. :) I can see it being a doc bug, but I think consolidate is working as intended (just, it doesn't state its prereqs well enough).

@benesch

benesch commented Mar 28, 2020

Copy link
Copy Markdown
Contributor

TimelyDataflow/differential-dataflow#264

The docs on consolidate state:

/// This method will sort `vec[offset..]` and then consolidate runs with the same first
/// element of a pair by accumulating the second elements of the pairs. Should the final
/// accumulation be zero, the element is discarded.

And yet if the slice has one element like [("a", 0)] consolidate will fail to remove that element.

@benesch benesch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well do this too!

@frankmcsherry frankmcsherry merged commit 20820c0 into MaterializeInc:master Mar 28, 2020
@cuongdo

cuongdo commented Mar 31, 2020

Copy link
Copy Markdown
Contributor

@frankmcsherry could you add a relevant release note to doc/user/release-notes.md?

@frankmcsherry

Copy link
Copy Markdown
Contributor Author

Tbh, I'm not sure what to add. There was a bug that caused a crash, and it doesn't crash any more. There should be zero user-facing changes, other than fewer crashes. Any tips?

@frankmcsherry

Copy link
Copy Markdown
Contributor Author

Looking at the description of the file, I don't think this qualifies as a "major bug fix". I'm up for being corrected on that, though.

@frankmcsherry frankmcsherry deleted the fix_2464 branch February 11, 2022 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants