sql: fix incorrect distsql plan caused by nil filter #44307
Conversation
|
This change is |
RaduBerinde
force-pushed
the
fix-empty-filter
branch
from
c74d95f
to
0d869b7
Compare
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @solongordon)
pkg/sql/logictest/testdata/logic_test/select, line 647 at r1 (raw file):
# but is statically evaluated to true when building the filterNode). statement ok CREATE TABLE t44203(c0 BOOL);
[nit]: unnecessary semicolon.
pkg/sql/physicalplan/physical_plan.go, line 638 at r1 (raw file):
filter.LocalExpr = tree.NewTypedAndExpr( post.Filter.LocalExpr, filter.LocalExpr,
Will this work in case when filter.LocalExpr is nil?
I also think that this distinction between .Expr and .LocalExpr was put in-place by Jordan to improve the performance, but I guess it is ok to "write" .Expr even if we're using .LocalExpr since it'll happen only once during planning, right?
RaduBerinde
force-pushed
the
fix-empty-filter
branch
from
0d869b7
to
3496f70
Compare
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/sql/logictest/testdata/logic_test/select, line 647 at r1 (raw file):
Previously, yuzefovich wrote…
[nit]: unnecessary semicolon.
Done.
pkg/sql/physicalplan/physical_plan.go, line 638 at r1 (raw file):
Previously, yuzefovich wrote…
Will this work in case when
filter.LocalExpris nil?I also think that this distinction between
.Exprand.LocalExprwas put in-place by Jordan to improve the performance, but I guess it is ok to "write".Expreven if we're using.LocalExprsince it'll happen only once during planning, right?
Wasn't sure about this, added the if back in.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status:
pkg/sql/physicalplan/physical_plan.go, line 638 at r1 (raw file):
Previously, RaduBerinde wrote…
Wasn't sure about this, added the
ifback in.
Personally, I'd keep this whole code chunk untouched (because we might not want to fill in .Expr when .LocalExpr is non-nil - I feel like there is an implicit assumption that only of them is actually set), but it's up to you.
In some cases, filters that are not constant-folded by the optimizer can be evaluated statically to true when building the filterNode. In this case we create a filterNode with a nil filter, which is ill handled by the distsql planner. This change elides the filterNode in this case, and fixes up the distsl code to error out in this case. Release note (bug fix): fixed incorrect plans in very rare cases involving filters that aren't constant folded in the optimizer but that can be evaluated statically when running a given query.
RaduBerinde
force-pushed
the
fix-empty-filter
branch
from
3496f70
to
d909b0e
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/physicalplan/physical_plan.go, line 638 at r1 (raw file):
Previously, yuzefovich wrote…
Personally, I'd keep this whole code chunk untouched (because we might not want to fill in
.Exprwhen.LocalExpris non-nil - I feel like there is an implicit assumption that only of them is actually set), but it's up to you.
Ok, added a comment.
|
bors r+ |
44307: sql: fix incorrect distsql plan caused by nil filter r=RaduBerinde a=RaduBerinde In some cases, filters that are not constant-folded by the optimizer can be evaluated statically to true when building the filterNode. In this case we create a filterNode with a nil filter, which is ill handled by the distsql planner. This change elides the filterNode in this case, and fixes up the distsl code to error out in this case. Release note (bug fix): fixed incorrect plans in very rare cases involving filters that aren't constant folded in the optimizer but that can be evaluated statically when running a given query. Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Build succeeded |
In some cases, filters that are not constant-folded by the optimizer
can be evaluated statically to true when building the filterNode. In
this case we create a filterNode with a nil filter, which is ill
handled by the distsql planner.
This change elides the filterNode in this case, and fixes up the
distsl code to error out in this case.
Release note (bug fix): fixed incorrect plans in very rare cases
involving filters that aren't constant folded in the optimizer but
that can be evaluated statically when running a given query.