Skip to content

Maintain negative inputs for the Distinct operator#7279

Closed
antiguru wants to merge 2 commits into
MaterializeInc:mainfrom
antiguru:negate_input
Closed

Maintain negative inputs for the Distinct operator#7279
antiguru wants to merge 2 commits into
MaterializeInc:mainfrom
antiguru:negate_input

Conversation

@antiguru

@antiguru antiguru commented Jul 1, 2021

Copy link
Copy Markdown
Member

Signed-off-by: Moritz Hoffmann mh@materialize.com

@ruchirK

ruchirK commented Jul 1, 2021

Copy link
Copy Markdown
Contributor

Hey Moritz sorry for the drive-by review I know this PR is still a draft. Iff these changes are meant for this ticket https://github.com/MaterializeInc/database-issues/issues/2266 then I'm pretty sure you can achieve what its asking for without a feedback edge. build_bucketed_stage is the function where we apply this optimization for hierarchical reduce

@antiguru antiguru requested a review from ruchirK July 1, 2021 12:35
@antiguru

antiguru commented Jul 1, 2021

Copy link
Copy Markdown
Member Author

Ruchir, much appreciated! I rewrote the patch to not use a feedback loop but rather compute the negation and concat it further on directly. Some tests seem to pass...

Comment thread src/dataflow/src/render/reduce.rs
Comment thread src/dataflow/src/render/reduce.rs
@frankmcsherry

Copy link
Copy Markdown
Contributor

Sorry for the drive by, and I should have said this earlier, but: one bit of fall out that would happen here is that Distinct would no longer present an arrangement for use downstream. I don't know if that is especially bad news, but it is some sort of cost. I should have said as much in the ticket I think (I was thinking more about the Threshold operator, who's output is less interesting).

@frankmcsherry

Copy link
Copy Markdown
Contributor

Maybe one take is that both implementations are valuable, and that we should flesh out the reduce plan to explicitly indicate which one we want (e.g. so that if the planner notices that we would use the arrangement, we can stay with the current implementation, and if it notice that we would not use the arrangement it can go with the new implementation).

@antiguru antiguru requested a review from frankmcsherry July 1, 2021 12:42
/// to combine and present results in the appropriate order. If we
/// were only asked to compute a single aggregation, we can skip
/// that step and return the arragement provided by computing the aggregation
/// that step and return the arrangement provided by computing the aggregation

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.

hahaha

Comment thread src/dataflow/src/render/reduce.rs
antiguru added 2 commits July 2, 2021 09:16
Signed-off-by: Moritz Hoffmann <mh@materialize.com>
Signed-off-by: Moritz Hoffmann <mh@materialize.com>
@antiguru

antiguru commented Jul 6, 2021

Copy link
Copy Markdown
Member Author

Closing this PR in favor of #7287.

@antiguru antiguru closed this Jul 6, 2021
@antiguru antiguru deleted the negate_input branch October 29, 2021 07:44
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