Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Join order optimizer asan bug Follow up #11794

Merged

Conversation

Tmonster
Copy link
Contributor

Follow up to #11719

This PR removes the JoinNode class completely. In addition, it removes a lot of other dead code that should have been removed a while ago. This solution passes a const reference to the dynamic programming table of plans to the query_graph_manager so that plan reconstruction can directly reference the dynamic programming table. The reconstruction code can then lookup the left and right JoinRelationSets by looking into the DP table.

@duckdb-draftbot duckdb-draftbot marked this pull request as draft April 24, 2024 07:33
@Tmonster Tmonster marked this pull request as ready for review April 24, 2024 12:52
@Tmonster Tmonster changed the title Join order optimizer asan bug refactor Join order optimizer asan bug Follow up Apr 24, 2024
@Tmonster Tmonster requested review from Mytherin and lnkuiper and removed request for Mytherin April 24, 2024 13:47
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.

Looks great after the refactor! I just have two remarks, and then this is ready to go:

//! Tries to emit a potential join candidate pair. Returns false if too many pairs have already been emitted,
//! cancelling the dynamic programming step.
bool TryEmitPair(JoinRelationSet &left, JoinRelationSet &right, const vector<reference<NeighborInfo>> &info);

unique_ptr<JoinNode> CreateJoinNodeFromDPJoinNode(DPJoinNode dp_node);
// unique_ptr<JoinNode> CreateJoinNodeFromDPJoinNode(DPJoinNode dp_node);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this comment be removed?

return res;
}
const reference_map_t<JoinRelationSet, unique_ptr<DPJoinNode>> &PlanEnumerator::GetPlans() const {
// optional_ptr<reference_map_t<JoinRelationSet, unique_ptr<DPJoinNode>>> res = &plans;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this comment be removed?

@duckdb-draftbot duckdb-draftbot marked this pull request as draft April 25, 2024 09:34
@Tmonster Tmonster marked this pull request as ready for review April 25, 2024 09:35
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.

LGTM now, thanks!

@Mytherin Mytherin merged commit b91d89a into duckdb:main Apr 30, 2024
48 of 49 checks passed
@Mytherin
Copy link
Collaborator

Thanks!

github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request May 5, 2024
Merge pull request duckdb/duckdb#11794 from Tmonster/join_order_optimizer_asan_bug_and_refactor
Merge pull request duckdb/duckdb#11863 from Tishj/synchronize_test_requirements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants