sql: remove filter from scanNode#51493
Conversation
|
This change is |
948eef9 to
0cd44e9
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewed 34 of 34 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)
pkg/sql/distsql_physical_planner.go, line 471 at r1 (raw file):
rec := canDistribute // Check if we are doing a full scan. if n.isFull {
I wonder whether we should change the recommendation of the scan distribution when performing partial scan, probably based on the estimated row count and some threshold setting, what do you think? I guess it is orthogonal to this PR, so just thinking out loud.
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsql_physical_planner.go, line 471 at r1 (raw file):
Previously, yuzefovich wrote…
I wonder whether we should change the recommendation of the scan distribution when performing partial scan, probably based on the estimated row count and some threshold setting, what do you think? I guess it is orthogonal to this PR, so just thinking out loud.
Yeah I'd expect it to be a threshold on the estimated row count
Remove the filter from scanNode. It doesn't provide any benefit anymore and it just causes confusion (e.g. soft limit handling). In addition, we want EXPLAIN to map to exec.Factory calls as much as possible; in this case we want the filter to be shown as a separate node. Release note (sql change): EXPLAIN (PLAN) now shows any filter on a scan as a separate `filter` node.
0cd44e9 to
892b63e
Compare
|
bors r+ |
Canceled (will resume) |
Build succeeded |
Remove the filter from scanNode. It doesn't provide any benefit
anymore and it just causes confusion (e.g. soft limit handling). In
addition, we want EXPLAIN to map to exec.Factory calls as much as
possible; in this case we want the filter to be shown as a separate
node.
Release note (sql change): EXPLAIN (PLAN) now shows any filter on a
scan as a separate
filternode.