Skip to content

Commit

Permalink
Merge cockroachdb#28774 cockroachdb#29213
Browse files Browse the repository at this point in the history 
28774: opt: fix building of aggregates with outer columns r=rytaft a=rytaft

This commit fixes a bug in which aggregates containing
outer columns were associated with the wrong scope.

For example, consider this query:
```
  SELECT (SELECT max(a.x) FROM b) FROM a;
```
Previously, the inner (subquery) scope was selected as the grouping scope
for `max(a.x)`. However, this was incorrect. The outer scope should
have been selected instead.

In order to fix this issue, this commit makes significant changes
to the way queries are built by the `optbuilder`. It is difficult
to determine whether a particular select clause needs aggregation without
first building the arguments to the aggregates to see whether there are outer
columns. Therefore, this commit makes the following change: before any
expressions in the `SELECT` list are built, all aggregates in the `SELECT`,
`DISTINCT ON`, `ORDER BY`, and `HAVING` clauses are replaced with an `aggregateInfo`.
The `aggregateInfo` contains the function definition, fully built arguments, and
output column of the aggregate.

In order to avoid traversing the expression tree multiple times, the entire
semantic analysis and type checking phase is completed before
building any non-aggregates for these clauses.

This commit avoids allocating slices for analyzed expressions by
instead creating them as columns directly in the scope which will
use them. It later makes another pass through the columns to build them.
    
This and various other performance improvements make the fix for
correlated aggregates perform at parity with the master branch.

Master branch:
```
name                       time/op
Phases/kv-read/OptBuild    18.8µs ± 4%
Phases/planning1/OptBuild  8.06µs ± 3%
Phases/planning2/OptBuild  20.3µs ± 5%
Phases/planning3/OptBuild  22.0µs ± 3%
Phases/planning4/OptBuild  24.1µs ± 4%
Phases/planning5/OptBuild  22.2µs ± 8%
Phases/planning6/OptBuild  36.8µs ± 2%
```
Correlated aggs:
```
name                       time/op
Phases/kv-read/OptBuild    18.4µs ± 3%
Phases/planning1/OptBuild  7.92µs ± 1%
Phases/planning2/OptBuild  20.6µs ± 4%
Phases/planning3/OptBuild  22.2µs ± 4%
Phases/planning4/OptBuild  24.1µs ± 3%
Phases/planning5/OptBuild  21.4µs ± 1%
Phases/planning6/OptBuild  36.3µs ± 2%
```

Fixes cockroachdb#12703

Release note: None

29213: cli: Fix support for deprecated --http-host flag r=a-robinson a=a-robinson

It looks like we accidentally stopped registering the --http-host flag
in the cli package's init function recently, which completely removed
support for --http-host rather than the intended effect of deprecating
(but continuing to allow) it.

Also make the handling of deprecated/alias flags more consistent with
each other while I'm here.

Release note (bug fix): Fix support for --http-host flag that was broken
in previous 2.1 beta releases.

Co-authored-by: Rebecca Taft <[email protected]>
Co-authored-by: Alex Robinson <[email protected]>
  • Loading branch information
@rytaft @a-robinson
3 people committed Aug 28, 2018
3 parents 1393ad2 + cabde5d + 484af30 commit 50ff8dc
Show file tree
Hide file tree
Showing 54 changed files with 2,150 additions and 1,359 deletions.
8 changes: 5 additions & 3 deletions pkg/cli/flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,23 +271,25 @@ func init() {
VarFlag(f, aliasStrVar{&serverListenPort}, cliflags.ServerPort)
if markDeprecated {
_ = f.MarkDeprecated(cliflags.ServerHost.Name, "use --listen-addr/--advertise-addr instead.")
_ = f.MarkDeprecated(cliflags.ServerPort.Name, "use --listen-addr instead.")
_ = f.MarkDeprecated(cliflags.ServerPort.Name, "use --listen-addr=...:<port> instead.")
}

VarFlag(f, addrSetter{&serverAdvertiseAddr, &serverAdvertisePort}, cliflags.AdvertiseAddr)
VarFlag(f, aliasStrVar{&serverAdvertiseAddr}, cliflags.AdvertiseHost)
VarFlag(f, aliasStrVar{&serverAdvertisePort}, cliflags.AdvertisePort)
if markDeprecated {
_ = f.MarkDeprecated(cliflags.AdvertiseHost.Name, "use --advertise-addr instead.")
_ = f.MarkDeprecated(cliflags.AdvertisePort.Name, "use --advertise-addr=...:<port> instead.")
}

StringFlag(f, &serverAdvertisePort, cliflags.AdvertisePort, serverAdvertisePort)
VarFlag(f, &localityAdvertiseHosts, cliflags.LocalityAdvertiseAddr)
if markDeprecated {
_ = f.MarkDeprecated(cliflags.AdvertisePort.Name, "use --advertise-addr=...:<port> instead.")
}

VarFlag(f, addrSetter{&serverHTTPAddr, &serverHTTPPort}, cliflags.ListenHTTPAddr)
StringFlag(f, &serverHTTPPort, cliflags.ListenHTTPPort, serverHTTPPort)
VarFlag(f, aliasStrVar{&serverHTTPAddr}, cliflags.ListenHTTPAddrAlias)
VarFlag(f, aliasStrVar{&serverHTTPPort}, cliflags.ListenHTTPPort)
if markDeprecated {
_ = f.MarkDeprecated(cliflags.ListenHTTPAddrAlias.Name, "use --http-addr instead.")
_ = f.MarkDeprecated(cliflags.ListenHTTPPort.Name, "use --http-addr=...:<port> instead.")
Expand Down
7 changes: 7 additions & 0 deletions pkg/cli/flags_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"time"

"fmt"

"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/cli/cliflags"
"github.com/cockroachdb/cockroach/pkg/server/status"
Expand Down Expand Up @@ -294,6 +295,12 @@ func TestHttpHostFlagValue(t *testing.T) {
}{
{[]string{"start", "--" + cliflags.ListenHTTPAddr.Name, "127.0.0.1"}, "127.0.0.1:" + base.DefaultHTTPPort},
{[]string{"start", "--" + cliflags.ListenHTTPAddr.Name, "192.168.0.111"}, "192.168.0.111:" + base.DefaultHTTPPort},
// confirm --http-host still works
{[]string{"start", "--" + cliflags.ListenHTTPAddrAlias.Name, "127.0.0.1"}, "127.0.0.1:" + base.DefaultHTTPPort},
{[]string{"start", "--" + cliflags.ListenHTTPAddr.Name, ":12345", "--" + cliflags.ListenHTTPAddrAlias.Name, "192.168.0.111"}, "192.168.0.111:12345"},
// confirm --http-port still works
{[]string{"start", "--" + cliflags.ListenHTTPPort.Name, "12345"}, ":12345"},
{[]string{"start", "--" + cliflags.ListenHTTPAddr.Name, "192.168.0.111", "--" + cliflags.ListenHTTPPort.Name, "12345"}, "192.168.0.111:12345"},
// confirm hostnames will work
{[]string{"start", "--" + cliflags.ListenHTTPAddr.Name, "my.host.name"}, "my.host.name:" + base.DefaultHTTPPort},
{[]string{"start", "--" + cliflags.ListenHTTPAddr.Name, "myhostname"}, "myhostname:" + base.DefaultHTTPPort},
Expand Down
4 changes: 2 additions & 2 deletions pkg/cli/interactive_tests/test_flags.tcl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,15 @@ end_test

start_test "Check that server --port causes a deprecation warning."
send "$argv start --insecure --port=26257\r"
eexpect "port has been deprecated, use --listen-addr instead."
eexpect "port has been deprecated, use --listen-addr=...:<port> instead."
eexpect "node starting"
interrupt
eexpect ":/# "
end_test

start_test "Check that server --advertise-port causes a deprecation warning."
send "$argv start --insecure --advertise-port=12345\r"
eexpect "advertise-port has been deprecated, use --advertise-addr"
eexpect "advertise-port has been deprecated, use --advertise-addr=...:<port> instead."
eexpect "node starting"
interrupt
eexpect ":/# "
Expand Down
28 changes: 14 additions & 14 deletions pkg/sql/opt/exec/execbuilder/testdata/aggregate
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,11 @@ render · · (min
│ aggregate 5 sum(column1) · ·
│ aggregate 6 stddev(column1) · ·
│ aggregate 7 variance(column1) · ·
│ aggregate 8 bool_and(column11) · ·
│ aggregate 9 bool_or(column11) · ·
│ aggregate 10 xor_agg(column14) · ·
│ aggregate 8 bool_and(column10) · ·
│ aggregate 9 bool_or(column10) · ·
│ aggregate 10 xor_agg(column13) · ·
│ scalar · · ·
└── render · · (column1 int, column11 bool, column14 bytes) ·
└── render · · (column1 int, column10 bool, column13 bytes) ·
│ render 0 (1)[int] · ·
│ render 1 (true)[bool] · ·
│ render 2 ('\x01')[bytes] · ·
Expand All @@ -124,12 +124,12 @@ EXPLAIN (TYPES) SELECT count(*), k+v AS r FROM kv GROUP BY k+v
----
render · · (count int, r int) ·
│ render 0 (agg0)[int] · ·
│ render 1 (column5)[int] · ·
└── group · · (column5 int, agg0 int) ·
│ aggregate 0 column5 · ·
│ render 1 (column6)[int] · ·
└── group · · (column6 int, agg0 int) ·
│ aggregate 0 column6 · ·
│ aggregate 1 count_rows() · ·
│ group by @1 · ·
└── render · · (column5 int) ·
└── render · · (column6 int) ·
│ render 0 ((k)[int] + (v)[int])[int] · ·
└── scan · · (k int, v int) ·
· table kv@primary · ·
Expand Down Expand Up @@ -868,11 +868,11 @@ EXPLAIN (VERBOSE) SELECT min(a) AS m FROM foo GROUP BY @1
----
render · · (m) ·
│ render 0 agg0 · ·
└── group · · (column4, agg0) ·
│ aggregate 0 column4 · ·
└── group · · (column5, agg0) ·
│ aggregate 0 column5 · ·
│ aggregate 1 min(a) · ·
│ group by @1 · ·
└── render · · (column4, a) ·
└── render · · (column5, a) ·
│ render 0 a · ·
│ render 1 a · ·
└── scan · · (a) ·
Expand All @@ -884,11 +884,11 @@ EXPLAIN (VERBOSE) SELECT min(a) AS m FROM foo GROUP BY @2
----
render · · (m) ·
│ render 0 agg0 · ·
└── group · · (column4, agg0) ·
│ aggregate 0 column4 · ·
└── group · · (column5, agg0) ·
│ aggregate 0 column5 · ·
│ aggregate 1 min(a) · ·
│ group by @1 · ·
└── render · · (column4, a) ·
└── render · · (column5, a) ·
│ render 0 b · ·
│ render 1 a · ·
└── scan · · (a, b) ·
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/opt/exec/execbuilder/testdata/distsql_agg
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ https://cockroachdb.github.io/distsqlplan/decode.html#eJy8lF-L4jAUxd_3U8h9WiFg80
query T
SELECT url FROM [EXPLAIN (DISTSQL) SELECT max(a), min(b) FROM data HAVING min(b) > 2]
----
https://cockroachdb.github.io/distsqlplan/decode.html#eJyslE-L2zAQxe_9FGFOuyBYS3ayWZ28l4IPm5SQQ6H1QbUGY3AsI8nQEvzdi21I7JDIhuioP0-_92bEnKFSEnfihAb4L6BAgAGBEAhEQGANKYFaqwyNUbq7MggS-Rd4QKCo6sZ22ymBTGkEfgZb2BKBw1H8KfGAQqJ-C4CARCuKssfUujgJ_S-WwgogsG8sX8WUxAzSloBq7PVVY0WOwGlLlpM_81xjLqzSb-sp-CvZvcTsFQh8ff58ienrQyB7CLxymkppiRrlBJK2bks0uOOJXjz17r4XpUXdFWX1uwmCEFeMc57sjpdqMRLTh-bDiXm6vE_Ub59myKOibPz0iS2PyvxGnSGPor77iRoujxr6jTpDHkXd-okaLY8a-Y06Qx5F_fA_aO4AD2hqVRm8GTj3Xw66QYQyx2FqGdXoDH9olfWYYbnvdf2GRGOHUzoskmo46gyOxdQpZhMxvRUzN3kGHTrVkVscPeN77RRv3OTNM-R3p3jrJm-fIX-4exXMfBP3J7tlp-23_wEAAP__CmLBNw==
https://cockroachdb.github.io/distsqlplan/decode.html#eJyslEGLozAUx-_7KeSdZiAwJtpOJyfnsuBh2qX0sLDrIWseIlgjSYRdit99UQ_V0kahOSbxn9_7-cK7QK0k7sUZDfBfQIEAAwIREIiBwAYyAo1WORqjdP_JGEjlX-AhgbJuWttvZwRypRH4BWxpKwQOJ_GnwiMKifotBAISrSirAdPo8iz0v0QKK4DAobU8SChJGGQdAdXa663GigKB046sJ38WhcZCWKXfNnPw1-fPl4S-AoGvdP-SsNeHQPYQeOW0tdISNcoZJOvcJdFwqSYC38vKouZBwoLfbRhGGDDOebo_Paw3mtVL17eG-m3NAnnyH7Z-WsPWqzK_qgvkieq7H9VovWrkV3WBPFHd-VGN16vGflUXyBPVD_-z5Q7wiKZRtcGbGXP_5rCfPSgLHAeVUa3O8YdW-YAZl4chN2xINHY8peMircejvsBpmDrDbBamt2HmJi-gI2c6dofjZ-reOMNbN3n7DPndGd65ybtnyB_uXoULz8T9yG7ZWfftfwAAAP__UEC-Jw==

query T
SELECT url FROM [EXPLAIN (DISTSQL) SELECT DISTINCT (a) FROM data]
Expand Down Expand Up @@ -178,7 +178,7 @@ https://cockroachdb.github.io/distsqlplan/decode.html#eJyUkkFrs0AQhu_fr5A5JWQ-4q
query T
SELECT url FROM [EXPLAIN (DISTSQL) SELECT sum(a), sum(b), sum(c) FROM data GROUP BY d HAVING sum(a+b) > 10]
----
https://cockroachdb.github.io/distsqlplan/decode.html#eJzUlt9r4kAQx9_vrwjz1HJ7NJMftuYpcj9AsHp47cNxJyU1gxVsVjYbuFL8348kh0a97qSsgj65MflkZvfzZcgrZDKlYfJMOUS_AEGABwJ8EBCAgBAmApZKTinPpSofqYF--gciV8A8Wxa6_HsiYCoVQfQKeq4XBBHcJY8LGlOSkrpyQUBKOpkvqjJLNX9O1EucJjoBAWPKUlKRE-PH2BNOjMKpfn3hxAFMVgJkoTeFcp3MCCJcifbN9GYzRbNES3UVbvcSl9e94c-H4ejuYXg_GFzE4SUI-HF_exHjeuWtV_56FVzuNLep9_jiPCX5004phMlqswHvzQ1s3lNkUqWkKN16U_UWwxbR3Su8u0d2Z_9W1Vl8my90Jchzfheu65ODbhRFX75-7t_2BiBgVOio9BUHIg7fNOYfdsND-UkurzDcefL_tYOt2tg-unj06DLNNLx2TjO6eMjo4hlE12sfH-_o8WGaaZzt9WnGxztkfLwziI_fPj7-0ePDNNM425vTjI9_yPj4ZxCfoH18gqPHh2mmcbbd04xPcMj4BGcQH-ZLeUz5UmY5tfqqcssDoXRG9enlslBT-q7ktCpTX44qrvojpVzXd7G-6Gf1rbLBJoy7MDZhbwvG98EdG7hrA6NV3xiaac943r4Z9s2yOmZbgZEOzXBoo9oMM6rNMKPaDHOqGZpR3bFRfW2Eb8yybmxkmWFGlhlmZJlhThZDM7K6NrKQmaLcGLWbo3aD1G6SWo5Su1mKVsMUmWkaMNL2xum7pJlpTpqZ5qSZaVYag3PS9oaqUdpk9eFvAAAA__-A5poC
https://cockroachdb.github.io/distsqlplan/decode.html#eJzUlt9r4kAQx9_vrwjz1HJ7NJNNrOYpcj9AsHp47cNxJyU1gxVsVjYJXCn-70cS0Kh3OymroE8m6iczO58vQ94gVQmN4hfKIPwFCAI8ECBBgA8CApgKWGk1oyxTuvxLDQySPxC6AhbpqsjLr6cCZkoThG-QL_IlQQj38dOSJhQnpG9cEJBQHi-WVZmVXrzE-jVK4jwGARNKE9KhE-HHyBNOhMKpPqVwIh-mawGqyLeFsjyeE4S4Fu2b6c_nmuZxrvRNsNtLVN73Rz8fR-P7x9HDcHgVBdcg4MfD3VXkba7k5srfXOH1XnPbek-vznOcPe-VQpiutwfw_nuA7XOKVOmENCU7T6qeYjgiugeF98-ILc9YzeLbYplXggLnd-G6khx0wzD88vXz4K4_BAHjIg9LZ5EUBmPyuAceqU9qdYPB3j__XdvfqY3to4snjy7TTMNr5zyji8eMLl5AdL328fFOHh-mmcZsb88zPt4x4-NdQHxk-_jIk8eHaaYx2-55xkceMz7yAuLjt4-Pf_L4MM00Zts7z_j4x4yPfwHxYd6UJ5StVJpRq7cqtxwIJXOqp5epQs_ou1azqkx9O6646ouEsrz-FeubQVr_VDbYhHEfxibs7cD4PrhjA_dsYLTqGwMz7RnnLc2wNMvqmG35Rjoww4GNajPMqDbDjGozzKlmaEZ1x0b1rRHummV1bWSZYUaWGWZkmWFOFkMzsno2spDZotwatdujdovUbpNarlK7XYpWyxSZbeoz0g7W6bukmWlOmpnmpJlpVhqDc9IOlqpR2nT94W8AAAD__4QBmgI=

query T
SELECT url FROM [EXPLAIN (DISTSQL) SELECT avg(a+b), c FROM data GROUP BY c, d HAVING c = d]
Expand Down
Loading

0 comments on commit 50ff8dc

Please sign in to comment.