Skip to content

Commit d20da2a

Browse files
craig[bot]yuzefovich
andcommitted
147389: importer: uncomment test case in TestImportDefaultNextVal r=yuzefovich a=yuzefovich It referenced a now-stale TODO. Epic: None Release note: None 147944: execinfrapb: break dependency on parser r=yuzefovich a=yuzefovich This commit breaks dependency of `sql/execinfrapb` package on `sql/parser` package. It does the following mechanical changes: - `colinfo.CompareDatums` is moved towards its single call site - `init*FromAST` methods of window frame are moved to `sql` - `MakeEvalContext` is moved (and renamed) towards its single call site - `ExprHelper` and friends is moved into a new `execinfra/execexpr` package (we didn't want to move it into `execinfra` to not introduce a dependency of `colexecagg` on `execinfra`) - `ExprFmtCtxBase` is moved towards its single call site. All changes are no-ops (modulo some renaming and unexporting). Informs: #79357. Epic: None Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]>
3 parents 1c2c92b + 953c039 + 41c49ea commit d20da2a

30 files changed

+400
-349
lines changed

pkg/BUILD.bazel

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -476,6 +476,7 @@ ALL_TESTS = [
476476
"//pkg/sql/doctor:doctor_test",
477477
"//pkg/sql/enum:enum_test",
478478
"//pkg/sql/evalcatalog:evalcatalog_disallowed_imports_test",
479+
"//pkg/sql/execinfra/execexpr:execexpr_test",
479480
"//pkg/sql/execinfra:execinfra_disallowed_imports_test",
480481
"//pkg/sql/execinfra:execinfra_test",
481482
"//pkg/sql/execinfrapb:execinfrapb_disallowed_imports_test",
@@ -1980,6 +1981,8 @@ GO_TARGETS = [
19801981
"//pkg/sql/enum:enum_test",
19811982
"//pkg/sql/evalcatalog:evalcatalog",
19821983
"//pkg/sql/execinfra/execagg:execagg",
1984+
"//pkg/sql/execinfra/execexpr:execexpr",
1985+
"//pkg/sql/execinfra/execexpr:execexpr_test",
19831986
"//pkg/sql/execinfra/execopnode:execopnode",
19841987
"//pkg/sql/execinfra/execreleasable:execreleasable",
19851988
"//pkg/sql/execinfra:execinfra",

pkg/sql/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -427,6 +427,7 @@ go_library(
427427
"//pkg/sql/evalcatalog",
428428
"//pkg/sql/execinfra",
429429
"//pkg/sql/execinfra/execagg",
430+
"//pkg/sql/execinfra/execexpr",
430431
"//pkg/sql/execinfra/execopnode",
431432
"//pkg/sql/execinfra/execreleasable",
432433
"//pkg/sql/execinfrapb",

pkg/sql/catalog/colinfo/BUILD.bazel

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ go_library(
2424
"//pkg/sql/pgwire/pgcode",
2525
"//pkg/sql/pgwire/pgerror",
2626
"//pkg/sql/sem/catconstants",
27-
"//pkg/sql/sem/eval",
2827
"//pkg/sql/sem/idxtype",
2928
"//pkg/sql/sem/tree",
3029
"//pkg/sql/sqlerrors",

pkg/sql/catalog/colinfo/ordering.go

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,7 @@ package colinfo
77

88
import (
99
"bytes"
10-
"context"
1110

12-
"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
1311
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
1412
"github.com/cockroachdb/cockroach/pkg/util/encoding"
1513
)
@@ -63,27 +61,3 @@ func (ordering ColumnOrdering) String(columns ResultColumns) string {
6361

6462
// NoOrdering is used to indicate an empty ColumnOrdering.
6563
var NoOrdering ColumnOrdering
66-
67-
// CompareDatums compares two datum rows according to a column ordering. Returns:
68-
// - 0 if lhs and rhs are equal on the ordering columns;
69-
// - less than 0 if lhs comes first;
70-
// - greater than 0 if rhs comes first.
71-
func CompareDatums(
72-
ctx context.Context, ordering ColumnOrdering, evalCtx *eval.Context, lhs, rhs tree.Datums,
73-
) int {
74-
for _, c := range ordering {
75-
// TODO(pmattis): This is assuming that the datum types are compatible. I'm
76-
// not sure this always holds as `CASE` expressions can return different
77-
// types for a column for different rows. Investigate how other RDBMs
78-
// handle this.
79-
if cmp, err := lhs[c.ColIdx].Compare(ctx, evalCtx, rhs[c.ColIdx]); err != nil {
80-
panic(err)
81-
} else if cmp != 0 {
82-
if c.Direction == encoding.Descending {
83-
cmp = -cmp
84-
}
85-
return cmp
86-
}
87-
}
88-
return 0
89-
}

pkg/sql/colexec/colbuilder/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ go_library(
3434
"//pkg/sql/colmem",
3535
"//pkg/sql/execinfra",
3636
"//pkg/sql/execinfra/execagg",
37+
"//pkg/sql/execinfra/execexpr",
3738
"//pkg/sql/execinfra/execreleasable",
3839
"//pkg/sql/execinfrapb",
3940
"//pkg/sql/row",

pkg/sql/colexec/colbuilder/execplan.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ import (
3636
"github.com/cockroachdb/cockroach/pkg/sql/colmem"
3737
"github.com/cockroachdb/cockroach/pkg/sql/execinfra"
3838
"github.com/cockroachdb/cockroach/pkg/sql/execinfra/execagg"
39+
"github.com/cockroachdb/cockroach/pkg/sql/execinfra/execexpr"
3940
"github.com/cockroachdb/cockroach/pkg/sql/execinfra/execreleasable"
4041
"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
4142
"github.com/cockroachdb/cockroach/pkg/sql/row"
@@ -1896,7 +1897,7 @@ func processExpr(
18961897
if expr.LocalExpr != nil {
18971898
return expr.LocalExpr, nil
18981899
}
1899-
return execinfrapb.DeserializeExpr(ctx, expr, typs, semaCtx, evalCtx)
1900+
return execexpr.DeserializeExpr(ctx, expr, typs, semaCtx, evalCtx)
19001901
}
19011902

19021903
// planAndMaybeWrapFilter plans a filter. If the filter is unsupported, it is

pkg/sql/distsql_plan_window.go

Lines changed: 206 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,18 @@ package sql
88
import (
99
"context"
1010

11+
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb"
1112
"github.com/cockroachdb/cockroach/pkg/sql/execinfra/execagg"
1213
"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
14+
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
15+
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
16+
"github.com/cockroachdb/cockroach/pkg/sql/rowenc"
1317
"github.com/cockroachdb/cockroach/pkg/sql/rowexec"
18+
"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
19+
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
20+
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treewindow"
1421
"github.com/cockroachdb/cockroach/pkg/sql/types"
22+
"github.com/cockroachdb/cockroach/pkg/util/duration"
1523
"github.com/cockroachdb/errors"
1624
)
1725

@@ -49,12 +57,207 @@ func createWindowFnSpec(
4957
}
5058
if funcInProgress.frame != nil {
5159
// funcInProgress has a custom window frame.
52-
frameSpec := execinfrapb.WindowerSpec_Frame{}
53-
if err := frameSpec.InitFromAST(ctx, funcInProgress.frame, planCtx.EvalContext()); err != nil {
60+
frameSpec := &execinfrapb.WindowerSpec_Frame{}
61+
if err := initFrameFromAST(ctx, frameSpec, funcInProgress.frame, planCtx.EvalContext()); err != nil {
5462
return execinfrapb.WindowerSpec_WindowFn{}, outputType, err
5563
}
56-
funcInProgressSpec.Frame = &frameSpec
64+
funcInProgressSpec.Frame = frameSpec
5765
}
5866

5967
return funcInProgressSpec, outputType, nil
6068
}
69+
70+
// initFrameFromAST initializes the spec based on tree.WindowFrame. It will
71+
// evaluate offset expressions if present in the frame.
72+
func initFrameFromAST(
73+
ctx context.Context,
74+
spec *execinfrapb.WindowerSpec_Frame,
75+
f *tree.WindowFrame,
76+
evalCtx *eval.Context,
77+
) error {
78+
if err := initModeFromAST(&spec.Mode, f.Mode); err != nil {
79+
return err
80+
}
81+
if err := initExclusionFromAST(&spec.Exclusion, f.Exclusion); err != nil {
82+
return err
83+
}
84+
return initBoundsFromAST(ctx, &spec.Bounds, f.Bounds, f.Mode, evalCtx)
85+
}
86+
87+
func initModeFromAST(
88+
spec *execinfrapb.WindowerSpec_Frame_Mode, w treewindow.WindowFrameMode,
89+
) error {
90+
switch w {
91+
case treewindow.RANGE:
92+
*spec = execinfrapb.WindowerSpec_Frame_RANGE
93+
case treewindow.ROWS:
94+
*spec = execinfrapb.WindowerSpec_Frame_ROWS
95+
case treewindow.GROUPS:
96+
*spec = execinfrapb.WindowerSpec_Frame_GROUPS
97+
default:
98+
return errors.AssertionFailedf("unexpected WindowFrameMode")
99+
}
100+
return nil
101+
}
102+
103+
func initExclusionFromAST(
104+
spec *execinfrapb.WindowerSpec_Frame_Exclusion, e treewindow.WindowFrameExclusion,
105+
) error {
106+
switch e {
107+
case treewindow.NoExclusion:
108+
*spec = execinfrapb.WindowerSpec_Frame_NO_EXCLUSION
109+
case treewindow.ExcludeCurrentRow:
110+
*spec = execinfrapb.WindowerSpec_Frame_EXCLUDE_CURRENT_ROW
111+
case treewindow.ExcludeGroup:
112+
*spec = execinfrapb.WindowerSpec_Frame_EXCLUDE_GROUP
113+
case treewindow.ExcludeTies:
114+
*spec = execinfrapb.WindowerSpec_Frame_EXCLUDE_TIES
115+
default:
116+
return errors.AssertionFailedf("unexpected WindowerFrameExclusion")
117+
}
118+
return nil
119+
}
120+
121+
func initBoundTypeFromAST(
122+
spec *execinfrapb.WindowerSpec_Frame_BoundType, bt treewindow.WindowFrameBoundType,
123+
) error {
124+
switch bt {
125+
case treewindow.UnboundedPreceding:
126+
*spec = execinfrapb.WindowerSpec_Frame_UNBOUNDED_PRECEDING
127+
case treewindow.OffsetPreceding:
128+
*spec = execinfrapb.WindowerSpec_Frame_OFFSET_PRECEDING
129+
case treewindow.CurrentRow:
130+
*spec = execinfrapb.WindowerSpec_Frame_CURRENT_ROW
131+
case treewindow.OffsetFollowing:
132+
*spec = execinfrapb.WindowerSpec_Frame_OFFSET_FOLLOWING
133+
case treewindow.UnboundedFollowing:
134+
*spec = execinfrapb.WindowerSpec_Frame_UNBOUNDED_FOLLOWING
135+
default:
136+
return errors.AssertionFailedf("unexpected WindowFrameBoundType")
137+
}
138+
return nil
139+
}
140+
141+
// If offset exprs are present, we evaluate them and save the encoded results
142+
// in the spec.
143+
func initBoundsFromAST(
144+
ctx context.Context,
145+
spec *execinfrapb.WindowerSpec_Frame_Bounds,
146+
b tree.WindowFrameBounds,
147+
m treewindow.WindowFrameMode,
148+
evalCtx *eval.Context,
149+
) error {
150+
if b.StartBound == nil {
151+
return errors.Errorf("unexpected: Start Bound is nil")
152+
}
153+
spec.Start = execinfrapb.WindowerSpec_Frame_Bound{}
154+
if err := initBoundTypeFromAST(&spec.Start.BoundType, b.StartBound.BoundType); err != nil {
155+
return err
156+
}
157+
if b.StartBound.HasOffset() {
158+
typedStartOffset := b.StartBound.OffsetExpr.(tree.TypedExpr)
159+
dStartOffset, err := eval.Expr(ctx, evalCtx, typedStartOffset)
160+
if err != nil {
161+
return err
162+
}
163+
if dStartOffset == tree.DNull {
164+
return pgerror.Newf(pgcode.NullValueNotAllowed, "frame starting offset must not be null")
165+
}
166+
switch m {
167+
case treewindow.ROWS:
168+
startOffset := int64(tree.MustBeDInt(dStartOffset))
169+
if startOffset < 0 {
170+
return pgerror.Newf(pgcode.InvalidWindowFrameOffset, "frame starting offset must not be negative")
171+
}
172+
spec.Start.IntOffset = uint64(startOffset)
173+
case treewindow.RANGE:
174+
if neg, err := isNegative(ctx, evalCtx, dStartOffset); err != nil {
175+
return err
176+
} else if neg {
177+
return pgerror.Newf(pgcode.InvalidWindowFrameOffset, "invalid preceding or following size in window function")
178+
}
179+
typ := dStartOffset.ResolvedType()
180+
spec.Start.OffsetType = execinfrapb.DatumInfo{Encoding: catenumpb.DatumEncoding_VALUE, Type: typ}
181+
var buf []byte
182+
var a tree.DatumAlloc
183+
datum := rowenc.DatumToEncDatum(typ, dStartOffset)
184+
buf, err = datum.Encode(typ, &a, catenumpb.DatumEncoding_VALUE, buf)
185+
if err != nil {
186+
return err
187+
}
188+
spec.Start.TypedOffset = buf
189+
case treewindow.GROUPS:
190+
startOffset := int64(tree.MustBeDInt(dStartOffset))
191+
if startOffset < 0 {
192+
return pgerror.Newf(pgcode.InvalidWindowFrameOffset, "frame starting offset must not be negative")
193+
}
194+
spec.Start.IntOffset = uint64(startOffset)
195+
}
196+
}
197+
198+
if b.EndBound != nil {
199+
spec.End = &execinfrapb.WindowerSpec_Frame_Bound{}
200+
if err := initBoundTypeFromAST(&spec.End.BoundType, b.EndBound.BoundType); err != nil {
201+
return err
202+
}
203+
if b.EndBound.HasOffset() {
204+
typedEndOffset := b.EndBound.OffsetExpr.(tree.TypedExpr)
205+
dEndOffset, err := eval.Expr(ctx, evalCtx, typedEndOffset)
206+
if err != nil {
207+
return err
208+
}
209+
if dEndOffset == tree.DNull {
210+
return pgerror.Newf(pgcode.NullValueNotAllowed, "frame ending offset must not be null")
211+
}
212+
switch m {
213+
case treewindow.ROWS:
214+
endOffset := int64(tree.MustBeDInt(dEndOffset))
215+
if endOffset < 0 {
216+
return pgerror.Newf(pgcode.InvalidWindowFrameOffset, "frame ending offset must not be negative")
217+
}
218+
spec.End.IntOffset = uint64(endOffset)
219+
case treewindow.RANGE:
220+
if neg, err := isNegative(ctx, evalCtx, dEndOffset); err != nil {
221+
return err
222+
} else if neg {
223+
return pgerror.Newf(pgcode.InvalidWindowFrameOffset, "invalid preceding or following size in window function")
224+
}
225+
typ := dEndOffset.ResolvedType()
226+
spec.End.OffsetType = execinfrapb.DatumInfo{Encoding: catenumpb.DatumEncoding_VALUE, Type: typ}
227+
var buf []byte
228+
var a tree.DatumAlloc
229+
datum := rowenc.DatumToEncDatum(typ, dEndOffset)
230+
buf, err = datum.Encode(typ, &a, catenumpb.DatumEncoding_VALUE, buf)
231+
if err != nil {
232+
return err
233+
}
234+
spec.End.TypedOffset = buf
235+
case treewindow.GROUPS:
236+
endOffset := int64(tree.MustBeDInt(dEndOffset))
237+
if endOffset < 0 {
238+
return pgerror.Newf(pgcode.InvalidWindowFrameOffset, "frame ending offset must not be negative")
239+
}
240+
spec.End.IntOffset = uint64(endOffset)
241+
}
242+
}
243+
}
244+
245+
return nil
246+
}
247+
248+
// isNegative returns whether offset is negative.
249+
func isNegative(ctx context.Context, evalCtx *eval.Context, offset tree.Datum) (bool, error) {
250+
switch o := offset.(type) {
251+
case *tree.DInt:
252+
return *o < 0, nil
253+
case *tree.DDecimal:
254+
return o.Negative, nil
255+
case *tree.DFloat:
256+
return *o < 0, nil
257+
case *tree.DInterval:
258+
cmp, err := o.Compare(ctx, evalCtx, &tree.DInterval{Duration: duration.Duration{}})
259+
return cmp < 0, err
260+
default:
261+
panic("unexpected offset type")
262+
}
263+
}

pkg/sql/distsql_running.go

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ import (
4242
"github.com/cockroachdb/cockroach/pkg/sql/rowexec"
4343
"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
4444
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
45+
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
4546
"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
4647
"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
4748
"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
@@ -373,6 +374,19 @@ func (c *cancelFlowsCoordinator) addFlowsToCancel(
373374
}
374375
}
375376

377+
// serializeEvalContext serializes some of the fields of a eval.Context into a
378+
// execinfrapb.EvalContext proto.
379+
func serializeEvalContext(evalCtx *eval.Context) execinfrapb.EvalContext {
380+
sessionDataProto := evalCtx.SessionData().SessionData
381+
sessiondata.MarshalNonLocal(evalCtx.SessionData(), &sessionDataProto)
382+
return execinfrapb.EvalContext{
383+
SessionData: sessionDataProto,
384+
StmtTimestampNanos: evalCtx.StmtTimestamp.UnixNano(),
385+
TxnTimestampNanos: evalCtx.TxnTimestamp.UnixNano(),
386+
TestingKnobsForceProductionValues: evalCtx.TestingKnobs.ForceProductionValues,
387+
}
388+
}
389+
376390
// setupFlows sets up all the flows specified in flows using the provided state.
377391
// It will first attempt to set up the gateway flow (whose output is the
378392
// DistSQLReceiver provided) and - if successful - will proceed to setting up
@@ -454,7 +468,7 @@ func (dsp *DistSQLPlanner) setupFlows(
454468
setupReq.EvalContext.SessionData.VectorizeMode = evalCtx.SessionData().VectorizeMode
455469
} else {
456470
// In distributed plans populate some extra state.
457-
setupReq.EvalContext = execinfrapb.MakeEvalContext(evalCtx)
471+
setupReq.EvalContext = serializeEvalContext(evalCtx)
458472
if jobTag, ok := logtags.FromContext(ctx).GetTag("job"); ok {
459473
setupReq.JobTag = redact.SafeString(jobTag.ValueStr())
460474
}

pkg/sql/execinfra/BUILD.bazel

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ go_library(
5050
"//pkg/sql/catalog/tabledesc",
5151
"//pkg/sql/catalog/typedesc",
5252
"//pkg/sql/evalcatalog",
53+
"//pkg/sql/execinfra/execexpr",
5354
"//pkg/sql/execinfrapb",
5455
"//pkg/sql/pgwire/pgerror",
5556
"//pkg/sql/rowenc",
@@ -131,5 +132,6 @@ disallowed_imports_test(
131132
"//pkg/sql/row",
132133
"//pkg/sql/rowexec",
133134
"//pkg/sql/rowflow",
135+
"//pkg/sql/sem/builtins",
134136
],
135137
)

pkg/sql/execinfra/execagg/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ go_library(
66
importpath = "github.com/cockroachdb/cockroach/pkg/sql/execinfra/execagg",
77
visibility = ["//visibility:public"],
88
deps = [
9+
"//pkg/sql/execinfra/execexpr",
910
"//pkg/sql/execinfrapb",
1011
"//pkg/sql/sem/builtins",
1112
"//pkg/sql/sem/builtins/builtinsregistry",

0 commit comments

Comments
 (0)