fix(optimizer): Fix issues with join graph construction

[desmondcheongzx]

We make three fixes to join graph construction. With these changes, join ordering using the naive left deep join orderer produces correct results for TPCH queries.

Push up projections and filters only if they sit in between joins

Instead of pushing up all Projections and Filters until we hit an unreorderable node, we go down each linear chain of (reorderable) Projections and Filters until we hit a Join node, then push up the Projects and Filters we encountered along the way. If we hit an unreorderable node first, we simply treat the operator that sits at the top of the current linear chain as the relation to pass into the join.

For example, consider this query tree:

        //                InnerJoin (a = d)
        //                    /        \
        //                   /       Project
        //                  /        (d, quad <- double + double)
        //                 /               \
        //     InnerJoin (a = b)    InnerJoin (c = d)
        //        /        \           /         \
        // Scan(a)      Scan(b)   Filter        Scan(d)
        //                        (c < 5)
        //                           |
        //                        Project
        //                        (c <- c_prime, double <- c_prime + c_prime)
        //                           |
        //                        Filter
        //                        (c_prime > 0)
        //                           |
        //                        Scan(c_prime)

In between InnerJoin(c=d) and Scan(c_prime) there are Filter and Project nodes. Since there is no join below InnerJoin(c=d), we take the Filter(c<5) operator as the relation to pass into the join (as opposed to using Scan(c_prime) and pushing up the Projects and Filters above it).

If a relation needs to rename one column, make sure the other columns are also selected

Previously, a relation might need to have a column renamed. However when we did this, we didn't select the other columns in the relation, causing some columns to be dropped prematurely.

Rename columns for relations even if they are not involved in join conditions

Previously, if we encountered a projection (e.g. a_prime <- a), we would apply this projection directly above the source relation if a_prime was involved in a join, e.g. Join(left_on="a_prime", ..). Now, we uniformly apply these projections regardless if the projection affects a join condition.

[codspeed-hq]

CodSpeed Performance Report

Merging #3668 will degrade performances by 30.82%

_{Comparing desmondcheongzx:fix-join-graph-building (7c578a8) with main (5702720)}

Summary

❌ 1 regressions
✅ 26 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	main	desmondcheongzx:fix-join-graph-building	Change
❌	test_iter_rows_first_row[100 Small Files]	180.9 ms	261.5 ms	-30.82%

[codecov]

Codecov Report

Attention: Patch coverage is 90.90909% with 15 lines in your changes missing coverage. Please review.

Project coverage is 77.80%. Comparing base (c932ec9) to head (7c578a8).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
...src/optimization/rules/reorder_joins/join_graph.rs	90.85%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3668      +/-   ##
==========================================
- Coverage   78.06%   77.80%   -0.27%     
==========================================
  Files         728      729       +1     
  Lines       89967    90557     +590     
==========================================
+ Hits        70236    70458     +222     
- Misses      19731    20099     +368     

Files with missing lines	Coverage Δ
.../rules/reorder_joins/naive_left_deep_join_order.rs	95.49% <100.00%> (ø)
...src/optimization/rules/reorder_joins/join_graph.rs	88.99% <90.85%> (+0.19%)	⬆️

... and 46 files with indirect coverage changes

[samster25]

should this be plan or curr_node?

[desmondcheongzx]

In this case it's intended to be plan because we just grab the node at the top of the linear chain. I guess process_leaf_relation is a misleading name. Changing the function name to add_relation and adding a comment explaining what's going on.

[samster25]

I think it would be less bug prone to have

let old_plan = plan;
and then you can modify plan freely instead of referring to cur_node

[samster25]

Does this do a full equality operation or just a Arc::ptr check?

[desmondcheongzx]

Ah for some reason I keep forgetting that Arc dereferences the inner value when doing eq. Will change this to ptr_eq

[samster25]

Let's have a better message here