Skip to content

Commit

Permalink
Merge #74236
Browse files Browse the repository at this point in the history 
74236: colfetcher,row: decode only needed columns when tracing is enabled r=yuzefovich a=yuzefovich

**colfetcher: minor optimization when decoding the value part**

Release note: None

**colfetcher: fix recent bug of using wrong column with tracing in some cases**

A couple of months ago when we taught the cFetcher to operate only on
needed columns, we refactored the meaning of `ColIdxMap` - now it
returns for the given ColumnID the ordinal of the column among all
needed columns whereas previously it would return the ordinal among all
columns in the table. In tracing we were relying on the old behavior to
append the column name to the key part, and we mistakenly used the
"needed" ordinal when accesing the "whole table" columns. This is now
fixed. I noticed it while working on the next commit.

Release note: None

**colfetcher,row: decode only needed columns when tracing is enabled**

In a recent change we made it so that the cFetcher would treat all
columns present in the index as "needed" when the KV tracing is enabled.
This led to some bugs (like reading of a virtual column that doesn't
exist in the index) but is also not very meaningful since the goal of
the tracing is to show what the cFetcher is doing without tracing. This
commit starts treating the KV tracing as the request to log all needed
operations without doing anything extra (like decoding unnecessary
columns).

A similar change is applied to the `row.Fetcher` as well.

Fixes: #73745.

Release note (sql change): The KV tracing of SQL queries (that could be
obtained with `\set auto_trace=on,kv`) has been adjusted slightly.
Previously, we would fully decode the key part of each key-value pair
even if some part of the key would not be decoded when tracing is
enabled. Now, we won't perform any extra decoding, and parts of the key
that aren't decoded are replaced with `?`.

**colfetcher: find needed columns upfront**

Previously, the two users of the `cFetcher` (`ColBatchScan` and
`ColIndexJoin`) had different ways of figuring out which columns are
actually needed to be fetched (that information is needed to not decode
parts of the KV pairs unnecessarily). In one case we explicitly
propagated the information through the spec, and in another we did some
pre-processing to find the information.

This overall was more complicated than necessary (because we had complex
code path for pruning "unaccessible" columns from the secondary index
while keeping track of the ordinal mapping) as well as suboptimal (in
some cases `TableReaderSpec.NeededColumns` contained ordinals of the
columns that weren't actually needed by the post-processing stage), so
this commit unifies both ways into one addressing both of these
shortcomings.

Now, in order to find the set of needed columns, we examine the
post-processing spec all the time, but we do it in an efficient manner:
- we make sure to deserialize render expressions early
- we use a `tree.Visitor` rather than a heavy-weight `ExprHelper`.

As a result, we can remove `TableReaderSpec.NeededColumns` field as well
as not decode some columns in a handful of cases.

Release note: None

**sql: log undecoded values as <undecoded> in tracing**

Previously, if the value part of a key-value pair wasn't needed, we
would log it as `NULL`. However, that can be confusing whether an actual
`NULL` value was decoded or not. This commit replaces the undecoded
values with `<undecoded>` reducing the confusion (given that the
likelihood of the value actually being equal to `<undecoded>` is
negligible).

Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
  • Loading branch information
@yuzefovich
craig[bot] and yuzefovich committed Jan 7, 2022
2 parents a217638 + 58609b8 commit 927a4a1
Show file tree
Hide file tree
Showing 31 changed files with 413 additions and 566 deletions.
58 changes: 29 additions & 29 deletions pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_query_behavior
View file
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
Scan /Table/60/1/"\xc0"/1/0
Scan /Table/60/1/"@"/1/0
Scan /Table/60/1/"\x80"/1/0
fetched: /regional_by_row_table/regional_by_row_table_pkey/'ap-southeast-2'/1/pk2/a/b/j -> /1/2/3/'{"a": "b"}'
fetched: /regional_by_row_table/regional_by_row_table_pkey/?/1/pk2/a/b/j -> /1/2/3/'{"a": "b"}'
output row: [1 1 2 3 '{"a": "b"}']

statement ok
Expand Down Expand Up @@ -449,7 +449,7 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
ORDER BY ordinality ASC
----
Scan /Table/60/1/"@"/1/0
fetched: /regional_by_row_table/regional_by_row_table_pkey/'ap-southeast-2'/1/pk2/a/b/j -> /1/2/3/'{"a": "b"}'
fetched: /regional_by_row_table/regional_by_row_table_pkey/?/1/pk2/a/b/j -> /1/2/3/'{"a": "b"}'
output row: [1 1 2 3 '{"a": "b"}']

statement ok
Expand All @@ -469,7 +469,7 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
----
Scan /Table/60/1/"@"/10/0
Scan /Table/60/1/"\x80"/10/0
fetched: /regional_by_row_table/regional_by_row_table_pkey/'ca-central-1'/10/pk2/a/b -> /10/11/12
fetched: /regional_by_row_table/regional_by_row_table_pkey/?/10/pk2/a/b -> /10/11/12
output row: [10 10 11 12 NULL]

statement ok
Expand Down Expand Up @@ -544,9 +544,9 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
ORDER BY ordinality ASC
----
Scan /Table/60/1/"@"/1/0, /Table/60/1/"@"/4/0
fetched: /regional_by_row_table/regional_by_row_table_pkey/'ap-southeast-2'/1/pk2/a/b/j -> /1/2/3/'{"a": "b"}'
fetched: /regional_by_row_table/regional_by_row_table_pkey/?/1/pk2/a/b/j -> /1/2/3/'{"a": "b"}'
output row: [1 1 2 3 '{"a": "b"}']
fetched: /regional_by_row_table/regional_by_row_table_pkey/'ap-southeast-2'/4/pk2/a/b/j -> /4/5/6/'{"c": "d"}'
fetched: /regional_by_row_table/regional_by_row_table_pkey/?/4/pk2/a/b/j -> /4/5/6/'{"c": "d"}'
output row: [4 4 5 6 '{"c": "d"}']

# Tests using locality optimized search for lookup joins (including foreign
Expand Down Expand Up @@ -616,9 +616,9 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
ORDER BY ordinality ASC
----
Scan /Table/62/1/"@"/10/0, /Table/62/1/"\x80"/10/0, /Table/62/1/"\xc0"/10/0
fetched: /child/child_pkey/'ap-southeast-2'/10/c_p_id -> /10
fetched: /child/child_pkey/?/10/c_p_id -> /10
Scan /Table/61/1/"@"/10/0, /Table/61/1/"\x80"/10/0, /Table/61/1/"\xc0"/10/0
fetched: /parent/parent_pkey/'ap-southeast-2'/10 -> NULL
fetched: /parent/parent_pkey/?/? -> <undecoded>

# Semi join with locality optimized search disabled.
query T
Expand Down Expand Up @@ -646,9 +646,9 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
ORDER BY ordinality ASC
----
Scan /Table/62/1/"@"/10/0, /Table/62/1/"\x80"/10/0, /Table/62/1/"\xc0"/10/0
fetched: /child/child_pkey/'ap-southeast-2'/10/c_p_id -> /10
fetched: /child/child_pkey/?/10/c_p_id -> /10
Scan /Table/61/1/"@"/10/0, /Table/61/1/"\x80"/10/0, /Table/61/1/"\xc0"/10/0
fetched: /parent/parent_pkey/'ap-southeast-2'/10 -> NULL
fetched: /parent/parent_pkey/?/? -> <undecoded>
output row: [10 10]

# Inner join with locality optimized search disabled.
Expand Down Expand Up @@ -677,9 +677,9 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
ORDER BY ordinality ASC
----
Scan /Table/62/1/"@"/10/0, /Table/62/1/"\x80"/10/0, /Table/62/1/"\xc0"/10/0
fetched: /child/child_pkey/'ap-southeast-2'/10/c_p_id -> /10
fetched: /child/child_pkey/?/10/c_p_id -> /10
Scan /Table/61/1/"@"/10/0, /Table/61/1/"\x80"/10/0, /Table/61/1/"\xc0"/10/0
fetched: /parent/parent_pkey/'ap-southeast-2'/10 -> NULL
fetched: /parent/parent_pkey/'ap-southeast-2'/10 -> <undecoded>
output row: [10 10 10]

# Left join with locality optimized search disabled.
Expand Down Expand Up @@ -708,9 +708,9 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
ORDER BY ordinality ASC
----
Scan /Table/62/1/"@"/10/0, /Table/62/1/"\x80"/10/0, /Table/62/1/"\xc0"/10/0
fetched: /child/child_pkey/'ap-southeast-2'/10/c_p_id -> /10
fetched: /child/child_pkey/?/10/c_p_id -> /10
Scan /Table/61/1/"@"/10/0, /Table/61/1/"\x80"/10/0, /Table/61/1/"\xc0"/10/0
fetched: /parent/parent_pkey/'ap-southeast-2'/10 -> NULL
fetched: /parent/parent_pkey/'ap-southeast-2'/10 -> <undecoded>
output row: [10 10 10]

statement ok
Expand Down Expand Up @@ -775,9 +775,9 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
ORDER BY ordinality ASC
----
Scan /Table/62/1/"@"/10/0
fetched: /child/child_pkey/'ap-southeast-2'/10/c_p_id -> /10
fetched: /child/child_pkey/?/10/c_p_id -> /10
Scan /Table/61/1/"@"/10/0
fetched: /parent/parent_pkey/'ap-southeast-2'/10 -> NULL
fetched: /parent/parent_pkey/?/? -> <undecoded>

statement ok
SET tracing = on,kv,results; SELECT * FROM child WHERE NOT EXISTS (SELECT * FROM parent WHERE p_id = c_p_id) AND c_id = 20; SET tracing = off
Expand All @@ -792,10 +792,10 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
----
Scan /Table/62/1/"@"/20/0
Scan /Table/62/1/"\x80"/20/0, /Table/62/1/"\xc0"/20/0
fetched: /child/child_pkey/'ca-central-1'/20/c_p_id -> /20
fetched: /child/child_pkey/?/20/c_p_id -> /20
Scan /Table/61/1/"@"/20/0
Scan /Table/61/1/"\x80"/20/0, /Table/61/1/"\xc0"/20/0
fetched: /parent/parent_pkey/'ca-central-1'/20 -> NULL
fetched: /parent/parent_pkey/?/? -> <undecoded>

# Semi join with locality optimized search enabled.
query T
Expand Down Expand Up @@ -851,9 +851,9 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
ORDER BY ordinality ASC
----
Scan /Table/62/1/"@"/10/0
fetched: /child/child_pkey/'ap-southeast-2'/10/c_p_id -> /10
fetched: /child/child_pkey/?/10/c_p_id -> /10
Scan /Table/61/1/"@"/10/0
fetched: /parent/parent_pkey/'ap-southeast-2'/10 -> NULL
fetched: /parent/parent_pkey/?/? -> <undecoded>
output row: [10 10]

statement ok
Expand All @@ -869,10 +869,10 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
----
Scan /Table/62/1/"@"/20/0
Scan /Table/62/1/"\x80"/20/0, /Table/62/1/"\xc0"/20/0
fetched: /child/child_pkey/'ca-central-1'/20/c_p_id -> /20
fetched: /child/child_pkey/?/20/c_p_id -> /20
Scan /Table/61/1/"@"/20/0
Scan /Table/61/1/"\x80"/20/0, /Table/61/1/"\xc0"/20/0
fetched: /parent/parent_pkey/'ca-central-1'/20 -> NULL
fetched: /parent/parent_pkey/?/? -> <undecoded>
output row: [20 20]

# Inner join with locality optimized search enabled.
Expand Down Expand Up @@ -929,9 +929,9 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
ORDER BY ordinality ASC
----
Scan /Table/62/1/"@"/10/0
fetched: /child/child_pkey/'ap-southeast-2'/10/c_p_id -> /10
fetched: /child/child_pkey/?/10/c_p_id -> /10
Scan /Table/61/1/"@"/10/0
fetched: /parent/parent_pkey/'ap-southeast-2'/10 -> NULL
fetched: /parent/parent_pkey/'ap-southeast-2'/10 -> <undecoded>
output row: [10 10 10]

statement ok
Expand All @@ -947,10 +947,10 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
----
Scan /Table/62/1/"@"/20/0
Scan /Table/62/1/"\x80"/20/0, /Table/62/1/"\xc0"/20/0
fetched: /child/child_pkey/'ca-central-1'/20/c_p_id -> /20
fetched: /child/child_pkey/?/20/c_p_id -> /20
Scan /Table/61/1/"@"/20/0
Scan /Table/61/1/"\x80"/20/0, /Table/61/1/"\xc0"/20/0
fetched: /parent/parent_pkey/'ca-central-1'/20 -> NULL
fetched: /parent/parent_pkey/'ca-central-1'/20 -> <undecoded>
output row: [20 20 20]

# Left join with locality optimized search enabled.
Expand Down Expand Up @@ -1007,9 +1007,9 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
ORDER BY ordinality ASC
----
Scan /Table/62/1/"@"/10/0
fetched: /child/child_pkey/'ap-southeast-2'/10/c_p_id -> /10
fetched: /child/child_pkey/?/10/c_p_id -> /10
Scan /Table/61/1/"@"/10/0
fetched: /parent/parent_pkey/'ap-southeast-2'/10 -> NULL
fetched: /parent/parent_pkey/'ap-southeast-2'/10 -> <undecoded>
output row: [10 10 10]

statement ok
Expand All @@ -1025,10 +1025,10 @@ SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
----
Scan /Table/62/1/"@"/20/0
Scan /Table/62/1/"\x80"/20/0, /Table/62/1/"\xc0"/20/0
fetched: /child/child_pkey/'ca-central-1'/20/c_p_id -> /20
fetched: /child/child_pkey/?/20/c_p_id -> /20
Scan /Table/61/1/"@"/20/0
Scan /Table/61/1/"\x80"/20/0, /Table/61/1/"\xc0"/20/0
fetched: /parent/parent_pkey/'ca-central-1'/20 -> NULL
fetched: /parent/parent_pkey/'ca-central-1'/20 -> <undecoded>
output row: [20 20 20]

query T
Expand Down
5 changes: 2 additions & 3 deletions pkg/sql/colexec/colbuilder/execplan_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,8 @@ func TestNewColOperatorExpectedTypeSchema(t *testing.T) {

desc := catalogkv.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "test", "t")
tr := execinfrapb.TableReaderSpec{
Table: *desc.TableDesc(),
Spans: make([]roachpb.Span, 1),
NeededColumns: []uint32{0},
Table: *desc.TableDesc(),
Spans: make([]roachpb.Span, 1),
}
var err error
tr.Spans[0].Key, err = randgen.TestingMakePrimaryIndexKey(desc, 0)
Expand Down
1 change: 0 additions & 1 deletion pkg/sql/colexec/colexecargs/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ go_library(
"//pkg/sql/colexecop",
"//pkg/sql/execinfra",
"//pkg/sql/execinfrapb",
"//pkg/sql/parser",
"//pkg/sql/sem/tree",
"//pkg/sql/types",
"//pkg/util/mon",
Expand Down
53 changes: 0 additions & 53 deletions pkg/sql/colexec/colexecargs/expr.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (
"sync"

"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
"github.com/cockroachdb/cockroach/pkg/sql/parser"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/types"
)
Expand Down Expand Up @@ -51,55 +50,3 @@ func (h *ExprHelper) ProcessExpr(
tempVars := tree.MakeIndexedVarHelper(&h.helper, len(typs))
return execinfrapb.DeserializeExpr(expr.Expr, h.SemaCtx, evalCtx, &tempVars)
}

// Remove unused warning.
var _ = findIVarsInRange

// findIVarsInRange searches Expr for presence of tree.IndexedVars with indices
// in range [start, end). It returns a slice containing all such indices.
func findIVarsInRange(expr execinfrapb.Expression, start int, end int) ([]uint32, error) {
res := make([]uint32, 0)
if start >= end {
return res, nil
}
var exprToWalk tree.Expr
if expr.LocalExpr != nil {
exprToWalk = expr.LocalExpr
} else {
e, err := parser.ParseExpr(expr.Expr)
if err != nil {
return nil, err
}
exprToWalk = e
}
visitor := ivarExpressionVisitor{ivarSeen: make([]bool, end)}
_, _ = tree.WalkExpr(visitor, exprToWalk)
for i := start; i < end; i++ {
if visitor.ivarSeen[i] {
res = append(res, uint32(i))
}
}
return res, nil
}

type ivarExpressionVisitor struct {
ivarSeen []bool
}

var _ tree.Visitor = &ivarExpressionVisitor{}

// VisitPre is a part of tree.Visitor interface.
func (i ivarExpressionVisitor) VisitPre(expr tree.Expr) (bool, tree.Expr) {
switch e := expr.(type) {
case *tree.IndexedVar:
if e.Idx < len(i.ivarSeen) {
i.ivarSeen[e.Idx] = true
}
return false, expr
default:
return true, expr
}
}

// VisitPost is a part of tree.Visitor interface.
func (i ivarExpressionVisitor) VisitPost(expr tree.Expr) tree.Expr { return expr }
1 change: 1 addition & 0 deletions pkg/sql/colfetcher/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ go_library(
"//pkg/sql/colencoding",
"//pkg/sql/colexec/colexecargs",
"//pkg/sql/colexec/colexecspan",
"//pkg/sql/colexec/colexecutils",
"//pkg/sql/colexecerror",
"//pkg/sql/colexecop",
"//pkg/sql/colmem",
Expand Down
Loading

0 comments on commit 927a4a1

Please sign in to comment.