Skip to content

Commit 6c3b66a

Browse files
committed
sql: clarify recent change around soft limits
AI review spotted a "bug" in a recent change around soft limits when that limit is negative. This is impossible by construction, so this commit clarifies the code a bit by switching to `uint64` in non-proto code. We also have "hard limit" that still remains `int64` due to one special case (I was being lazy so only left a TODO to clean this up). Release note: None
1 parent 1572ff3 commit 6c3b66a

File tree

6 files changed

+14
-9
lines changed

6 files changed

+14
-9
lines changed

pkg/sql/distsql_physical_planner.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -726,7 +726,7 @@ func checkSupportForPlanNode(
726726
var suffix string
727727
estimate := n.estimatedRowCount
728728
if n.softLimit != 0 && sd.UseSoftLimitForDistributeScan {
729-
estimate = uint64(n.softLimit)
729+
estimate = n.softLimit
730730
suffix = " (using soft limit)"
731731
}
732732
if estimate >= sd.DistributeScanRowCountThreshold {
@@ -2205,8 +2205,8 @@ func initTableReaderSpecTemplate(
22052205
var post execinfrapb.PostProcessSpec
22062206
if n.hardLimit != 0 {
22072207
post.Limit = uint64(n.hardLimit)
2208-
} else if n.softLimit != 0 {
2209-
s.LimitHint = n.softLimit
2208+
} else if softLimit := int64(n.softLimit); softLimit > 0 {
2209+
s.LimitHint = softLimit
22102210
}
22112211
return s, post, nil
22122212
}

pkg/sql/distsql_spec_exec_factory.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -337,8 +337,8 @@ func (e *distSQLSpecExecFactory) ConstructScan(
337337
post := execinfrapb.PostProcessSpec{}
338338
if params.HardLimit != 0 {
339339
post.Limit = uint64(params.HardLimit)
340-
} else if params.SoftLimit != 0 {
341-
trSpec.LimitHint = params.SoftLimit
340+
} else if softLimit := int64(params.SoftLimit); softLimit > 0 {
341+
trSpec.LimitHint = softLimit
342342
}
343343

344344
err = e.dsp.planTableReaders(

pkg/sql/opt/exec/execbuilder/relational.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -737,7 +737,7 @@ func (b *Builder) scanParams(
737737
sqltelemetry.IncrementPartitioningCounter(sqltelemetry.PartitionConstrainedScan)
738738
}
739739

740-
softLimit := reqProps.LimitHintInt64()
740+
softLimit := uint64(reqProps.LimitHintInt64())
741741
hardLimit := scan.HardLimit.RowCount()
742742
maxResults, maxResultsOk := b.indexConstraintMaxResults(scan, relProps)
743743

pkg/sql/opt/exec/explain/emit.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1319,7 +1319,7 @@ func (e *emitter) spansStr(table cat.Table, index cat.Index, scanParams exec.Sca
13191319
if scanParams.HardLimit != 0 {
13201320
return "LIMITED SCAN"
13211321
}
1322-
if scanParams.SoftLimit > 0 {
1322+
if scanParams.SoftLimit != 0 {
13231323
return "FULL SCAN (SOFT LIMIT)"
13241324
}
13251325
return "FULL SCAN"

pkg/sql/opt/exec/factory.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,12 +118,17 @@ type ScanParams struct {
118118
InvertedConstraint inverted.Spans
119119

120120
// If non-zero, the scan returns this many rows.
121+
//
122+
// Additionally, in plan-gist decoding path, this will be set to -1 to
123+
// indicate presence of a limit, regardless of its value.
124+
// TODO(yuzefovich): we could refactor this special case by adding an
125+
// additional boolean that would allow us to switch to using uint64.
121126
HardLimit int64
122127

123128
// If non-zero, the scan may still be required to return up to all its rows
124129
// (or up to the HardLimit if it is set, but can be optimized under the
125130
// assumption that only SoftLimit rows will be needed.
126-
SoftLimit int64
131+
SoftLimit uint64
127132

128133
Reverse bool
129134

pkg/sql/scan.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ type scanNode struct {
5252
// if non-zero, softLimit is an estimation that only this many rows might be
5353
// needed. It is a (potentially optimistic) "hint". If hardLimit is set
5454
// (non-zero), softLimit must be unset (zero).
55-
softLimit int64
55+
softLimit uint64
5656

5757
// See exec.Factory.ConstructScan.
5858
parallelize bool

0 commit comments

Comments
 (0)