Skip to content

Commit 852a1d9

Browse files
authored
fix: column pruning failure when applying associative law rules (#23206)
This PR fixes a column pruning failure issue caused by applying associative law optimization rules. When the query optimizer applies associative law transformations to reorder JOIN operations, the OnList conditions were not properly migrated to match the new JOIN structure, leading to remapping errors during column reference resolution. ### Problem When applying associative law rules (Rule 1, Rule 2, Rule 3) to transform JOIN structures like `A*(B*C)` to `(A*B)*C`, the OnList conditions that reference columns from moved tables were not migrated to the correct JOIN nodes. This caused column remapping failures because: 1. The JOIN structure was changed, but OnList conditions remained in their original positions 2. During column remapping phase, references to columns from moved tables could not be found in the expected context 3. This resulted in errors like "Column remapping failed: cannot find column reference" ### Solution The fix adds proper OnList condition migration logic in all three associative law rules: 1. **Added `migrateOnListConditions` function**: Moves conditions involving specified table tags from source node to destination node 2. **Added `checkExprInvolvesTags` function**: Checks if an expression references any of the specified table tags 3. **Updated Rule 1, Rule 2, Rule 3**: Each rule now properly migrates OnList conditions when transforming JOIN structures: - **Rule 1**: Migrates conditions involving table C to the outer join - **Rule 2**: Migrates conditions involving table A to the outer join - **Rule 3**: Migrates conditions involving B and C to their correct positions ### Testing Added a comprehensive test case `TestAssociativeLawRemapping` that: - Creates a scenario where associative law Rule 1 would be triggered - Verifies that the query executes successfully without remapping errors - Tests the exact scenario from the issue report Approved by: @aunjgr, @XuPeng-SH
1 parent d86da8b commit 852a1d9

File tree

9 files changed

+364
-4
lines changed

9 files changed

+364
-4
lines changed

pkg/sql/compile/sql_executor.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,13 +190,18 @@ func (s *sqlExecutor) getCompileContext(
190190
proc *process.Process,
191191
db string,
192192
lower int64) *compilerContext {
193-
return &compilerContext{
193+
cc := &compilerContext{
194194
ctx: ctx,
195195
defaultDB: db,
196196
engine: s.eng,
197197
proc: proc,
198198
lower: lower,
199199
}
200+
// For testing: check if a stats cache is provided in context
201+
if statsCache, ok := ctx.Value("test_stats_cache").(*plan.StatsCache); ok {
202+
cc.statsCache = statsCache
203+
}
204+
return cc
200205
}
201206

202207
func (s *sqlExecutor) adjustOptions(

pkg/sql/compile/sql_executor_context.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424

2525
"github.com/matrixorigin/matrixone/pkg/common/moerr"
2626
"github.com/matrixorigin/matrixone/pkg/defines"
27+
"github.com/matrixorigin/matrixone/pkg/logutil"
2728
planpb "github.com/matrixorigin/matrixone/pkg/pb/plan"
2829
pb "github.com/matrixorigin/matrixone/pkg/pb/statsinfo"
2930
"github.com/matrixorigin/matrixone/pkg/perfcounter"
@@ -137,6 +138,17 @@ func (c *compilerContext) Stats(obj *plan.ObjectRef, snapshot *plan.Snapshot) (*
137138
return nil, moerr.NewNoSuchTable(ctx, dbName, tableName)
138139
}
139140

141+
// For testing only: check if stats cache already has stats for this table
142+
// Only use cached stats if we're in a test environment (indicated by test_stats_cache in context)
143+
// If so, use the cached stats instead of fetching from engine
144+
if _, isTestEnv := c.GetContext().Value("test_stats_cache").(*plan.StatsCache); isTestEnv {
145+
tableID := table.GetTableID(ctx)
146+
if statsWrapper := c.statsCache.GetStatsInfo(tableID, false); statsWrapper != nil && statsWrapper.Stats != nil {
147+
logutil.Infof("use test env cached stats for table %s (tableID=%d)", tableName, tableID)
148+
return statsWrapper.Stats, nil
149+
}
150+
}
151+
140152
newCtx := perfcounter.AttachCalcTableStatsKey(ctx)
141153
statsInfo, err := table.Stats(newCtx, true)
142154
if err != nil {

pkg/sql/plan/associative_law.go

Lines changed: 122 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,65 @@
1414

1515
package plan
1616

17-
import "github.com/matrixorigin/matrixone/pkg/pb/plan"
17+
import (
18+
"fmt"
19+
20+
"github.com/matrixorigin/matrixone/pkg/pb/plan"
21+
)
22+
23+
// checkExprInvolvesTags checks if an expression references any of the specified tags
24+
func checkExprInvolvesTags(expr *plan.Expr, tagsMap map[int32]bool) bool {
25+
switch e := expr.Expr.(type) {
26+
case *plan.Expr_Col:
27+
return tagsMap[e.Col.RelPos]
28+
case *plan.Expr_F:
29+
for _, arg := range e.F.Args {
30+
if checkExprInvolvesTags(arg, tagsMap) {
31+
return true
32+
}
33+
}
34+
case *plan.Expr_W:
35+
if checkExprInvolvesTags(e.W.WindowFunc, tagsMap) {
36+
return true
37+
}
38+
for _, order := range e.W.OrderBy {
39+
if checkExprInvolvesTags(order.Expr, tagsMap) {
40+
return true
41+
}
42+
}
43+
}
44+
return false
45+
}
46+
47+
// migrateOnListConditions moves conditions involving specified tags from src to dst
48+
func migrateOnListConditions(src *plan.Node, dst *plan.Node, tagsMap map[int32]bool) {
49+
var kept []*plan.Expr
50+
for _, cond := range src.OnList {
51+
if checkExprInvolvesTags(cond, tagsMap) {
52+
dst.OnList = append(dst.OnList, cond)
53+
} else {
54+
kept = append(kept, cond)
55+
}
56+
}
57+
src.OnList = kept
58+
}
59+
60+
// getTableNameOrLabel returns table name from node's TableDef, or a label (A/B/C) if not available
61+
func (builder *QueryBuilder) getTableNameOrLabel(nodeID int32, label string) string {
62+
node := builder.qry.Nodes[nodeID]
63+
if node.TableDef != nil && node.TableDef.Name != "" {
64+
return node.TableDef.Name
65+
}
66+
return label
67+
}
68+
69+
// formatStatsInfo formats stats information for logging
70+
func formatStatsInfo(stats *plan.Stats) string {
71+
if stats == nil {
72+
return "stats=nil"
73+
}
74+
return fmt.Sprintf("sel=%.4f,outcnt=%.2f,tablecnt=%.2f", stats.Selectivity, stats.Outcnt, stats.TableCnt)
75+
}
1876

1977
// for A*(B*C), if C.sel>0.9 and B<C, change this to (A*B)*C
2078
func (builder *QueryBuilder) applyAssociativeLawRule1(nodeID int32) int32 {
@@ -43,6 +101,25 @@ func (builder *QueryBuilder) applyAssociativeLawRule1(nodeID int32) int32 {
43101
node.Children[1] = rightChild.NodeId
44102
return node.NodeId
45103
}
104+
105+
tagsC := builder.enumerateTags(NodeC.NodeId)
106+
// Migrate OnList: conditions involving C must move to outer join
107+
tagsCMap := make(map[int32]bool)
108+
for _, tag := range tagsC {
109+
tagsCMap[tag] = true
110+
}
111+
migrateOnListConditions(node, rightChild, tagsCMap)
112+
113+
// Record table names and stats after migration
114+
tableNameA := builder.getTableNameOrLabel(node.Children[0], "A")
115+
tableNameB := builder.getTableNameOrLabel(NodeB.NodeId, "B")
116+
tableNameC := builder.getTableNameOrLabel(NodeC.NodeId, "C")
117+
statsInfo := fmt.Sprintf("rule1: A=%s(stats:%s) B=%s(stats:%s) C=%s(stats:%s)",
118+
tableNameA, formatStatsInfo(builder.qry.Nodes[node.Children[0]].Stats),
119+
tableNameB, formatStatsInfo(NodeB.Stats),
120+
tableNameC, formatStatsInfo(NodeC.Stats))
121+
builder.optimizationHistory = append(builder.optimizationHistory, statsInfo)
122+
46123
rightChild.Children[0] = node.NodeId
47124
ReCalcNodeStats(rightChild.NodeId, builder, true, false, true)
48125
return rightChild.NodeId
@@ -67,6 +144,7 @@ func (builder *QueryBuilder) applyAssociativeLawRule2(nodeID int32) int32 {
67144
if NodeC.Stats.Selectivity > 0.5 {
68145
return nodeID
69146
}
147+
NodeA := builder.qry.Nodes[leftChild.Children[0]]
70148
NodeB := builder.qry.Nodes[leftChild.Children[1]]
71149
node.Children[0] = NodeB.NodeId
72150
determineHashOnPK(node.NodeId, builder)
@@ -75,6 +153,24 @@ func (builder *QueryBuilder) applyAssociativeLawRule2(nodeID int32) int32 {
75153
node.Children[0] = leftChild.NodeId
76154
return node.NodeId
77155
}
156+
157+
// Migrate OnList: conditions involving A must move to outer join
158+
tagsAMap := make(map[int32]bool)
159+
for _, tag := range builder.enumerateTags(NodeA.NodeId) {
160+
tagsAMap[tag] = true
161+
}
162+
migrateOnListConditions(node, leftChild, tagsAMap)
163+
164+
// Record table names and stats after migration
165+
tableNameA := builder.getTableNameOrLabel(NodeA.NodeId, "A")
166+
tableNameB := builder.getTableNameOrLabel(NodeB.NodeId, "B")
167+
tableNameC := builder.getTableNameOrLabel(NodeC.NodeId, "C")
168+
statsInfo := fmt.Sprintf("rule2: A=%s(stats:%s) B=%s(stats:%s) C=%s(stats:%s)",
169+
tableNameA, formatStatsInfo(NodeA.Stats),
170+
tableNameB, formatStatsInfo(NodeB.Stats),
171+
tableNameC, formatStatsInfo(NodeC.Stats))
172+
builder.optimizationHistory = append(builder.optimizationHistory, statsInfo)
173+
78174
leftChild.Children[1] = node.NodeId
79175
ReCalcNodeStats(leftChild.NodeId, builder, true, false, true)
80176
return leftChild.NodeId
@@ -110,6 +206,31 @@ func (builder *QueryBuilder) applyAssociativeLawRule3(nodeID int32) int32 {
110206
node.Children[0] = leftChild.NodeId
111207
return node.NodeId
112208
}
209+
210+
// Migrate OnList:
211+
// - node: move conditions involving B to leftChild
212+
// - leftChild: move conditions involving C to node
213+
tagsBMap := make(map[int32]bool)
214+
for _, tag := range builder.enumerateTags(NodeB.NodeId) {
215+
tagsBMap[tag] = true
216+
}
217+
tagsCMap := make(map[int32]bool)
218+
for _, tag := range builder.enumerateTags(NodeC.NodeId) {
219+
tagsCMap[tag] = true
220+
}
221+
migrateOnListConditions(node, leftChild, tagsBMap)
222+
migrateOnListConditions(leftChild, node, tagsCMap)
223+
224+
// Record table names and stats after migration
225+
tableNameA := builder.getTableNameOrLabel(NodeA.NodeId, "A")
226+
tableNameB := builder.getTableNameOrLabel(NodeB.NodeId, "B")
227+
tableNameC := builder.getTableNameOrLabel(NodeC.NodeId, "C")
228+
statsInfo := fmt.Sprintf("rule3: A=%s(stats:%s) B=%s(stats:%s) C=%s(stats:%s)",
229+
tableNameA, formatStatsInfo(NodeA.Stats),
230+
tableNameB, formatStatsInfo(NodeB.Stats),
231+
tableNameC, formatStatsInfo(NodeC.Stats))
232+
builder.optimizationHistory = append(builder.optimizationHistory, statsInfo)
233+
113234
leftChild.Children[0] = node.NodeId
114235
ReCalcNodeStats(leftChild.NodeId, builder, true, false, true)
115236
return leftChild.NodeId

pkg/sql/plan/join_order.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package plan
1616

1717
import (
18+
"fmt"
1819
"sort"
1920
"strings"
2021

@@ -204,6 +205,7 @@ func HasColExpr(expr *plan.Expr, pos int32) int32 {
204205
}
205206

206207
func (builder *QueryBuilder) determineJoinOrder(nodeID int32) int32 {
208+
originalNodeID := nodeID
207209
if builder.optimizerHints != nil && builder.optimizerHints.joinOrdering != 0 {
208210
return nodeID
209211
}
@@ -223,6 +225,9 @@ func (builder *QueryBuilder) determineJoinOrder(nodeID int32) int32 {
223225
}
224226

225227
leaves, conds := builder.gatherJoinLeavesAndConds(node, nil, nil)
228+
// Record middle: gathered leaves and conditions
229+
builder.optimizationHistory = append(builder.optimizationHistory,
230+
fmt.Sprintf("determineJoinOrder:middle (nodeID: %d, leaves: %d, conds: %d)", nodeID, len(leaves), len(conds)))
226231
newConds := deduceNewOnList(conds)
227232
conds = append(conds, newConds...)
228233
vertices := builder.getJoinGraph(leaves, conds)
@@ -347,6 +352,14 @@ func (builder *QueryBuilder) determineJoinOrder(nodeID int32) int32 {
347352
FilterList: conds,
348353
}, nil)
349354
}
355+
// Record after determineJoinOrder
356+
if nodeID != originalNodeID {
357+
builder.optimizationHistory = append(builder.optimizationHistory,
358+
fmt.Sprintf("determineJoinOrder:after (nodeID: %d -> %d, remainingConds: %d)", originalNodeID, nodeID, len(conds)))
359+
} else {
360+
builder.optimizationHistory = append(builder.optimizationHistory,
361+
fmt.Sprintf("determineJoinOrder:after (nodeID: %d, no change, remainingConds: %d)", nodeID, len(conds)))
362+
}
350363
return nodeID
351364
}
352365

pkg/sql/plan/opt_misc_test.go

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,12 @@
1515
package plan
1616

1717
import (
18+
"context"
19+
"testing"
20+
1821
"github.com/matrixorigin/matrixone/pkg/container/types"
1922
"github.com/matrixorigin/matrixone/pkg/pb/plan"
2023
"github.com/stretchr/testify/require"
21-
"testing"
2224
)
2325

2426
func TestRemapWindowClause(t *testing.T) {
@@ -43,7 +45,13 @@ func TestRemapWindowClause(t *testing.T) {
4345
Typ: plan.Type{},
4446
}
4547
colMap := make(map[[2]int32][2]int32)
46-
var b *QueryBuilder
48+
var b *QueryBuilder = &QueryBuilder{
49+
compCtx: &MockCompilerContext{
50+
ctx: context.Background(),
51+
},
52+
optimizationHistory: []string{"test optimization history"},
53+
}
4754
err := b.remapWindowClause(f, 1, 1, colMap, nil)
55+
t.Log(err)
4856
require.Error(t, err)
4957
}

pkg/sql/plan/pushdown.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,19 @@
1515
package plan
1616

1717
import (
18+
"fmt"
19+
1820
"github.com/matrixorigin/matrixone/pkg/catalog"
1921
"github.com/matrixorigin/matrixone/pkg/objectio"
2022
"github.com/matrixorigin/matrixone/pkg/pb/plan"
2123
"github.com/matrixorigin/matrixone/pkg/vectorindex/metric"
2224
)
2325

2426
func (builder *QueryBuilder) pushdownFilters(nodeID int32, filters []*plan.Expr, separateNonEquiConds bool) (int32, []*plan.Expr) {
27+
originalNodeID := nodeID
28+
// Record before pushdownFilters
29+
builder.optimizationHistory = append(builder.optimizationHistory,
30+
fmt.Sprintf("pushdownFilters:before (nodeID: %d, nodeType: %s, filters: %d)", nodeID, builder.qry.Nodes[nodeID].NodeType, len(filters)))
2531
node := builder.qry.Nodes[nodeID]
2632

2733
var canPushdown, cantPushdown []*plan.Expr
@@ -150,6 +156,9 @@ func (builder *QueryBuilder) pushdownFilters(nodeID int32, filters []*plan.Expr,
150156
}
151157

152158
case plan.Node_JOIN:
159+
// Record middle: processing JOIN node
160+
builder.optimizationHistory = append(builder.optimizationHistory,
161+
fmt.Sprintf("pushdownFilters:middle (nodeID: %d, JOIN, filters: %d, onList: %d)", nodeID, len(filters), len(node.OnList)))
153162
leftTags := make(map[int32]bool)
154163
for _, tag := range builder.enumerateTags(node.Children[0]) {
155164
leftTags[tag] = true
@@ -402,6 +411,9 @@ func (builder *QueryBuilder) pushdownFilters(nodeID int32, filters []*plan.Expr,
402411
node.Children[1] = childID
403412

404413
case plan.Node_UNION, plan.Node_UNION_ALL, plan.Node_MINUS, plan.Node_MINUS_ALL, plan.Node_INTERSECT, plan.Node_INTERSECT_ALL:
414+
// Record middle: processing UNION/MINUS/INTERSECT node
415+
builder.optimizationHistory = append(builder.optimizationHistory,
416+
fmt.Sprintf("pushdownFilters:middle (nodeID: %d, %s, filters: %d)", nodeID, node.NodeType, len(filters)))
405417
leftChild := builder.qry.Nodes[node.Children[0]]
406418
rightChild := builder.qry.Nodes[node.Children[1]]
407419
var canPushDownRight []*plan.Expr
@@ -457,6 +469,9 @@ func (builder *QueryBuilder) pushdownFilters(nodeID int32, filters []*plan.Expr,
457469
node.Children[0] = childID
458470

459471
case plan.Node_TABLE_SCAN, plan.Node_EXTERNAL_SCAN:
472+
// Record middle: processing TABLE_SCAN/EXTERNAL_SCAN node
473+
builder.optimizationHistory = append(builder.optimizationHistory,
474+
fmt.Sprintf("pushdownFilters:middle (nodeID: %d, %s, filters: %d)", nodeID, node.NodeType, len(filters)))
460475
for _, filter := range filters {
461476
if onlyContainsTag(filter, node.BindingTags[0]) {
462477
node.FilterList = append(node.FilterList, filter)
@@ -505,6 +520,14 @@ func (builder *QueryBuilder) pushdownFilters(nodeID int32, filters []*plan.Expr,
505520
}
506521
}
507522

523+
// Record after pushdownFilters
524+
if nodeID != originalNodeID {
525+
builder.optimizationHistory = append(builder.optimizationHistory,
526+
fmt.Sprintf("pushdownFilters:after (nodeID: %d -> %d, cantPushdown: %d)", originalNodeID, nodeID, len(cantPushdown)))
527+
} else {
528+
builder.optimizationHistory = append(builder.optimizationHistory,
529+
fmt.Sprintf("pushdownFilters:after (nodeID: %d, no change, cantPushdown: %d)", nodeID, len(cantPushdown)))
530+
}
508531
return nodeID, cantPushdown
509532
}
510533

pkg/sql/plan/query_builder.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ func NewQueryBuilder(queryType plan.Query_StatementType, ctx CompilerContext, is
9494
isPrepareStatement: isPrepareStatement,
9595
deleteNode: make(map[uint64]int32),
9696
skipStats: skipStats,
97+
optimizationHistory: make([]string, 0),
9798
}
9899
}
99100

@@ -193,6 +194,17 @@ func (builder *QueryBuilder) buildRemapErrorMessage(
193194
}
194195
}
195196

197+
// Optimization history
198+
if len(builder.optimizationHistory) > 0 {
199+
sb.WriteString("🔧 Optimization History:\n")
200+
for _, hist := range builder.optimizationHistory {
201+
sb.WriteString(fmt.Sprintf(" - %s\n", hist))
202+
}
203+
sb.WriteString("\n")
204+
} else {
205+
sb.WriteString("🔧 Optimization History: (no associatelaw applied)\n\n")
206+
}
207+
196208
// Available columns
197209
if len(colMap) > 0 {
198210
sb.WriteString("✅ Available Columns in Context:\n")

pkg/sql/plan/types.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,10 @@ type QueryBuilder struct {
193193
aggSpillMem int64
194194

195195
optimizerHints *OptimizerHints
196+
197+
// optimizationHistory records key optimization steps for debugging remap errors
198+
// Only records when optimizations actually change the plan structure
199+
optimizationHistory []string
196200
}
197201

198202
type OptimizerHints struct {

0 commit comments

Comments
 (0)