Skip to content

Conversation

Tmonster
Copy link
Contributor

Currently this logic is sprinkled within filter pull up and filter push down. The problem is it's not at every operator, and adding empty results across multiple files within the filter pullup/pushdown operator can make the code hard to maintain and debug.

This will unify the code and fix some fuzzer issues that are the cause of long execution times on the LHS of a join while a child on the RHS is empty . It can also help make other queries faster in a few cases.

One con is that some ConversionErrors will be hidden.

@duckdb-draftbot duckdb-draftbot marked this pull request as draft August 23, 2024 11:37
@Tmonster Tmonster marked this pull request as ready for review August 23, 2024 11:43
@Mytherin Mytherin changed the base branch from main to feature August 26, 2024 07:26
@Tmonster
Copy link
Contributor Author

Tmonster commented Sep 2, 2024

Main needs to be merged into feature, then the commit history should be cleaned up a little bit

@duckdb-draftbot duckdb-draftbot marked this pull request as draft September 2, 2024 13:16
@Tmonster Tmonster force-pushed the replace_window_with_empty_result_if_child_is_empty branch from 8821d57 to 8f5f4cb Compare September 19, 2024 11:33
@Tmonster Tmonster marked this pull request as ready for review September 20, 2024 07:54
@duckdb-draftbot duckdb-draftbot marked this pull request as draft September 20, 2024 08:41
@Tmonster Tmonster marked this pull request as ready for review September 20, 2024 08:42
@hannes
Copy link
Member

hannes commented Oct 2, 2024

Can you fix the conflict please?

@duckdb-draftbot duckdb-draftbot marked this pull request as draft October 2, 2024 11:40
@Tmonster Tmonster marked this pull request as ready for review October 2, 2024 12:02
@hannes hannes requested a review from lnkuiper October 2, 2024 12:09
Copy link
Contributor

@lnkuiper lnkuiper left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This looks good, I just have some minor comments:


namespace duckdb {

//! The Deliminator optimizer traverses the logical operator tree and removes any redundant DelimGets/DelimJoins
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you update this comment to describe this operator?

}

unique_ptr<LogicalOperator> Optimize(unique_ptr<LogicalOperator> op);
unique_ptr<LogicalOperator> PullUpEmptyJoinChildren(unique_ptr<LogicalOperator> op);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make PullUpEmptyJoinChildren private?

@duckdb-draftbot duckdb-draftbot marked this pull request as draft October 3, 2024 08:18
@Tmonster Tmonster marked this pull request as ready for review October 3, 2024 08:21
Copy link
Contributor

@lnkuiper lnkuiper left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, LGTM!

@Mytherin Mytherin merged commit 051b989 into duckdb:feature Oct 11, 2024
42 checks passed
@Mytherin
Copy link
Collaborator

Thanks! Looks great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants