remove unnecessary sort over merge joins #3108

jycor · 2025-07-17T21:34:08Z

This PR looks for Sort nodes over merge joins and removes them when applicable.

benchmarks: dolthub/dolt#9553
partially addresses: dolthub/dolt#8728

jycor · 2025-07-22T18:57:06Z

sql/analyzer/replace_sort.go

 	newNode, err := node.WithChildren(newChildren...)
 	if err != nil {
 		return nil, transform.SameTree, err
 	}
 	return newNode, transform.NewTree, nil
 }

+// buildReverseIndexedTable will attempt to take the lookup from an IndexedTableAccess, and return a new
+// IndexedTableAccess with the lookup reversed.
+func buildReverseIndexedTable(ctx *sql.Context, node sql.Node) (*plan.IndexedTableAccess, bool, error) {


This isn't currently used anywhere, but could be if we ever figure out how to properly reverse the index.

nicktobey · 2025-07-22T19:34:04Z

enginetest/queries/query_plans.go

-			" └─ Sort(one_pk.pk ASC)\n" +
-			"     └─ LeftOuterMergeJoin\n" +
+			" └─ Filter\n" +
+			"     ├─ (NOT(niltable.f IS NULL))\n" +


Why does do these plans add a filter that wasn't there before?

It didn't the diff is just offset: https://github.com/dolthub/go-mysql-server/pull/3108/files#diff-eae6109880e132a9197c2b536f62e9572fc34d8e7c65e957dd37f48657790a94L15426

nicktobey · 2025-07-22T19:35:42Z

enginetest/queries/query_plans.go

@@ -15418,129 +15468,64 @@ inner join pq on true
 			"             └─ columns: [i f]\n" +
 			"",
 	},
-	{
-		Query: `SELECT pk,i,f FROM one_pk LEFT JOIN niltable ON pk=i WHERE f IS NOT NULL ORDER BY 1`,


Why was this test deleted?

It wasn't the diff is just offset: https://github.com/dolthub/go-mysql-server/pull/3108/files#diff-eae6109880e132a9197c2b536f62e9572fc34d8e7c65e957dd37f48657790a94R15413

enginetest/queries/script_queries.go

nicktobey · 2025-07-22T19:38:11Z

enginetest/queries/script_queries.go

+				},
+			},
+			{
+				// The Sort node can be optimized out of this query, but currently is not


I'm not sure what's going on with this test. The comments talk about sort nodes, but the test only tests the output, not the plan? What specifically are we testing here?

That the output is correct.
I'm not sure if we should fill the script tests with plan tests. Plus, we already have plan tests.

sql/analyzer/replace_sort.go

nicktobey · 2025-07-22T19:42:05Z

sql/analyzer/replace_sort.go

@@ -20,9 +20,12 @@ func replaceIdxSort(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scope
 func replaceIdxSortHelper(ctx *sql.Context, scope *plan.Scope, node sql.Node, sortNode *plan.Sort) (sql.Node, transform.TreeIdentity, error) {
 	switch n := node.(type) {
 	case *plan.Sort:
-		sortNode = n // lowest parent sort node
+		// TODO: are there problems when there are multiple ORDER BYs?
+		if isValidSortFieldOrder(n.SortFields) {


It seems like if there's already an outer sort node, and then we encounter an inner sort node, then we might leave sortNode set to the outer one if the inner one isn't a valid sort field order. This probably can't cause an issue? (Since if you really had two sort nodes in a row the outer one would override the inner one), but I wanted to make sure we've thought about that possible code path.

sql/analyzer/replace_sort.go

nicktobey · 2025-07-23T00:34:45Z

sql/analyzer/replace_sort.go

@@ -184,6 +176,25 @@ func replaceIdxSortHelper(ctx *sql.Context, scope *plan.Scope, node sql.Node, so
 			if sameLeft && sameRight {
 				continue
 			}
+			// No need to check all SortField orders because of isValidSortFieldOrder
+			isReversed := sortNode.SortFields[0].Order == sql.Descending
+			// either left or right has been reversed


This comment confuses me. Why do we only use a reverse index if exactly one of the children as been modified? Why does the subsequent comment mention that "both Indexes must be reversed" but then only reverses one of them?

nicktobey · 2025-07-23T00:35:26Z

sql/analyzer/replace_sort.go

+				if err != nil {
+					return nil, transform.SameTree, err
+				}
+				// If we could not replace the IndexedTableAccess with a reversed one, result is same, so abandon


I'm not sure what "result is same" means here. Can this comment make more clear what's happening and what it means?

nicktobey · 2025-07-23T00:39:25Z

sql/analyzer/replace_sort.go

-	}
-	return newNode, true, nil
+func buildReverseIndexedTable(ctx *sql.Context, node sql.Node) (sql.Node, transform.TreeIdentity, error) {
+	return transform.Node(node, func(n sql.Node) (sql.Node, transform.TreeIdentity, error) {


Why do we call transform.Node here? What are the possible types of node? Is it not always an IndexedTableAccess?

It is possible for there to be Projects and Filters over the IndexedTableAccess.

sql/analyzer/replace_sort.go

sql/rowexec/merge_join.go

nicktobey · 2025-07-23T21:02:53Z

sql/rowexec/merge_join.go

@@ -216,6 +216,8 @@ func (i *mergeJoinIter) Next(ctx *sql.Context) (sql.Row, error) {
 			} else if err != nil {
 				return nil, err
 			}
+			// merge join assumes children area sorted in ascending order, so we need to invert the comparison to


area -> are

jycor marked this pull request as draft July 17, 2025 21:34

jycor force-pushed the james/merge branch 2 times, most recently from 2bb9f35 to 4a6dad3 Compare July 21, 2025 21:43

jycor marked this pull request as ready for review July 22, 2025 18:51

jycor commented Jul 22, 2025

View reviewed changes

nicktobey reviewed Jul 22, 2025

View reviewed changes

jycor mentioned this pull request Jul 23, 2025

implement reversed merge_joins dolthub/dolt#9553

Merged

nicktobey reviewed Jul 23, 2025

View reviewed changes

nicktobey approved these changes Jul 23, 2025

View reviewed changes

James Cor added 21 commits July 23, 2025 14:17

remove sort over merge joins

a50d2e9

fix

cca8c6f

add optimization and cleanup

cf407d1

regen plans

33b4e63

remove sort over merge joins

ab34283

fix

fdd8f70

add optimization and cleanup

29fc6a7

fix and some tests

71340ad

tmp

4223588

reversible merge join and more tests

8fe1c85

restore

5c4e9d0

fix typo

1d49935

last attempt

81392a5

no reverse

6204d2d

clean up

6390429

apply to more merge joins and regen plans

dd0b7d8

reapply reverse optimization

131ef6b

clean up

742bd1e

feedback

d62de1b

missed comment

b3bf3ab

typo

ed6fde7

jycor force-pushed the james/merge branch from 4f9519c to ed6fde7 Compare July 23, 2025 21:18

jycor merged commit 18eebe5 into main Jul 23, 2025
8 checks passed

jycor deleted the james/merge branch July 23, 2025 23:48

jycor mentioned this pull request Jul 24, 2025

Unneccessary sort operation when merge output is already known to be sorted. dolthub/dolt#8728

Open

BrewTestBot mentioned this pull request Jul 24, 2025

dolt 1.57.2 Homebrew/homebrew-core#231156

Merged

Uh oh!

remove unnecessary sort over merge joins #3108

remove unnecessary sort over merge joins #3108

Uh oh!

Conversation

jycor commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jycor Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jycor commented Jul 17, 2025 •

edited

Loading

jycor Jul 22, 2025 •

edited

Loading