Skip to content

Commit

Permalink
opt: don't mark st_makeline and st_extend as non-null
Browse files Browse the repository at this point in the history
`st_makeline` can take arguments of type `POINT`, `MULTIPOINT`, and
`LINESTRING`. Other types cause it to return null (even if the input is
non-null). Similarly, `st_extent` returns null when the input geometry is
non-null, but empty.

This commit prevents these two aggregate functions from being marked as
non-null, since doing so can lead to incorrect results in the cases
mentioned above.

Fixes #84957

Release justification: low-risk fix to bug that can lead to incorrect results

Release note (bug fix): Fixed a longstanding bug that could cause the optimizer
to produce an incorrect plan when aggregate functions `st_makeline` or
`st_extent` were called with invalid-type and empty inputs respectively.
  • Loading branch information
DrewKimball committed Aug 24, 2022
1 parent 5c55010 commit d63c4a2
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 4 deletions.
78 changes: 78 additions & 0 deletions pkg/sql/opt/memo/testdata/logprops/groupby
Original file line number Diff line number Diff line change
Expand Up @@ -354,3 +354,81 @@ project
└── corr [as=corr:9, type=float, outer=(1,2)]
├── variable: x:1 [type=int]
└── variable: y:2 [type=int]

# Regression test for #84957 - the aggregation output columns should not be
# marked as non-null.
#
# st_makeline with a non-null input of the wrong type (postgres returns null
# here rather than an error).
build
SELECT st_makeline('POLYGON((0 0, 1 0, 1 1, 0 1, 0 0))') FROM generate_series(1, 5) g(t) GROUP BY t;
----
project
├── columns: st_makeline:3(geometry)
├── immutable
├── prune: (3)
└── group-by (hash)
├── columns: generate_series:1(int) st_makeline:3(geometry)
├── grouping columns: generate_series:1(int)
├── immutable
├── key: (1)
├── fd: (1)-->(3)
├── prune: (3)
├── project
│ ├── columns: column2:2(geometry!null) generate_series:1(int)
│ ├── immutable
│ ├── fd: ()-->(2)
│ ├── prune: (1,2)
│ ├── project-set
│ │ ├── columns: generate_series:1(int)
│ │ ├── immutable
│ │ ├── values
│ │ │ ├── cardinality: [1 - 1]
│ │ │ ├── key: ()
│ │ │ └── tuple [type=tuple]
│ │ └── zip
│ │ └── function: generate_series [type=int, immutable]
│ │ ├── const: 1 [type=int]
│ │ └── const: 5 [type=int]
│ └── projections
│ └── const: '0103000000010000000500000000000000000000000000000000000000000000000000F03F0000000000000000000000000000F03F000000000000F03F0000000000000000000000000000F03F00000000000000000000000000000000' [as=column2:2, type=geometry]
└── aggregations
└── s-t-make-line [as=st_makeline:3, type=geometry, outer=(2)]
└── variable: column2:2 [type=geometry]

# st_extent with an empty (but non-null) input.
build
SELECT st_extent('0101000000000000000000F87F000000000000F87F') FROM generate_series(1, 5) g(t) GROUP BY t;
----
project
├── columns: st_extent:3(box2d)
├── immutable
├── prune: (3)
└── group-by (hash)
├── columns: generate_series:1(int) st_extent:3(box2d)
├── grouping columns: generate_series:1(int)
├── immutable
├── key: (1)
├── fd: (1)-->(3)
├── prune: (3)
├── project
│ ├── columns: column2:2(geometry!null) generate_series:1(int)
│ ├── immutable
│ ├── fd: ()-->(2)
│ ├── prune: (1,2)
│ ├── project-set
│ │ ├── columns: generate_series:1(int)
│ │ ├── immutable
│ │ ├── values
│ │ │ ├── cardinality: [1 - 1]
│ │ │ ├── key: ()
│ │ │ └── tuple [type=tuple]
│ │ └── zip
│ │ └── function: generate_series [type=int, immutable]
│ │ ├── const: 1 [type=int]
│ │ └── const: 5 [type=int]
│ └── projections
│ └── const: '0101000000000000000000F87F000000000000F87F' [as=column2:2, type=geometry]
└── aggregations
└── s-t-extent [as=st_extent:3, type=box2d, outer=(2)]
└── variable: column2:2 [type=geometry]
8 changes: 4 additions & 4 deletions pkg/sql/opt/operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -373,16 +373,16 @@ func AggregateIsNeverNullOnNonNullInput(op Operator) bool {
case AnyNotNullAggOp, ArrayAggOp, AvgOp, BitAndAggOp,
BitOrAggOp, BoolAndOp, BoolOrOp, ConcatAggOp, ConstAggOp,
ConstNotNullAggOp, CountOp, CountRowsOp, FirstAggOp,
JsonAggOp, JsonbAggOp, MaxOp, MinOp, SqrDiffOp, STMakeLineOp,
JsonAggOp, JsonbAggOp, MaxOp, MinOp, SqrDiffOp,
StringAggOp, SumOp, SumIntOp, XorAggOp, PercentileDiscOp, PercentileContOp,
JsonObjectAggOp, JsonbObjectAggOp, StdDevPopOp, STCollectOp, STExtentOp, STUnionOp,
JsonObjectAggOp, JsonbObjectAggOp, StdDevPopOp, STCollectOp, STUnionOp,
VarPopOp, CovarPopOp, RegressionAvgXOp, RegressionAvgYOp, RegressionSXXOp,
RegressionSXYOp, RegressionSYYOp, RegressionCountOp:
return true

case VarianceOp, StdDevOp, CorrOp, CovarSampOp, RegressionInterceptOp,
RegressionR2Op, RegressionSlopeOp:
// These aggregations return NULL if they are given a single not-NULL input.
RegressionR2Op, RegressionSlopeOp, STExtentOp, STMakeLineOp:
// These aggregations can return NULL even with non-null input values.
return false

default:
Expand Down

0 comments on commit d63c4a2

Please sign in to comment.