/cockroach

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release-2.1: add EXPLAIN(DISTSQL) support for subqueries; don't write/parse exprs in local mode #31284

Open
wants to merge 2 commits into
base: release-2.1
from

Conversation

Reviewers

@cockroachdb/sql-ccl-prs sql-ccl-prs

@cockroachdb/sql-rest-prs sql-rest-prs

@cockroachdb/sql-execution-prs sql-execution-prs

@cockroachdb/sql-planning-prs sql-planning-prs

@cockroachdb/distsql-prs distsql-prs

@solongordon solongordon

@asubiotto asubiotto

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@jordanlewis @cockroach-teamcity @asubiotto @arjunravinarayan
@jordanlewis
Member

jordanlewis commented Oct 11, 2018

I had to also backport "don't write/parse exprs in local mode" to get this to apply cleanly. I'm a little nervous about that, as it's a bit of a hefty change. That being said, it's a very good change that removes some of the last extra overhead that the DistSQL merge added - local expressions are no longer pointless serialized and re-parsed from text.

Backport:

  • 1/1 commits from "sql: don't write/parse exprs in local mode" (#30464)
  • 1/1 commits from "sql: add EXPLAIN(DISTSQL) support for subqueries" (#30857)

Please see individual PRs for details.

/cc @cockroachdb/release

jordanlewis and others added some commits Aug 25, 2018

@jordanlewis
sql: don't write/parse exprs in local mode 
Previously, expressions that needed to be used in processors were
written to a buffer with FmtParsable and re-parsed and type checked
with the parser, even if the processors were running in the local flow
mode. This was inefficient.

Now, we use a similar strategy to the "LocalPlanNode" system to avoid
this inefficiency. When we go to save an Expr to a processor spec, if
we're writing to a spec that will be inflated on the gateway, instead of
formatting the Expr to text, we save the Expr as an unserialized field
on the Go struct that corresponds to the Expression protobuf message.
Then, at inflation time, instead of parsing the text we would have
written, we simply return the Expr that's saved on that Go struct.

This should eliminate one of the major inefficiences introduced by the
distsql merge.

Release note: None
Verified
This commit was signed with a verified signature.
@jordanlewis jordanlewis Jordan Lewis
GPG key ID: CB92F5D43A08059A Learn about signing commits
4b17059
@arjunravinarayan @jordanlewis
sql: add EXPLAIN(DISTSQL) support for subqueries 
This currently does not provide the plans for the subqueries
themselves, just the outer query. But this no longer pops up the old
error message of "subqueries not supported", preventing a UX problem
where some queries are explainable and some are not.

Release note: None

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
Verified
This commit was signed with a verified signature.
@jordanlewis jordanlewis Jordan Lewis
GPG key ID: CB92F5D43A08059A Learn about signing commits
Loading status checks…
d4a78af

@jordanlewis jordanlewis requested review from solongordon and asubiotto Oct 11, 2018

@jordanlewis jordanlewis requested review from cockroachdb/distsql-prs as code owners Oct 11, 2018

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Oct 11, 2018

Member

This change is Reviewable
Member

cockroach-teamcity commented Oct 11, 2018

This change is Reviewable
@asubiotto

This comment has been minimized.

Show comment
Hide comment
@asubiotto

asubiotto Oct 15, 2018

Contributor

What's up with the TC failures?
Contributor

asubiotto commented Oct 15, 2018

What's up with the TC failures?
@jordanlewis

This comment has been minimized.

Show comment
Hide comment
@jordanlewis

jordanlewis Oct 15, 2018

Member

Glad you asked. I think I'm missing a backport. It's unfortunate that this backport required the local exprs branch in the first place. It would be much better to not require that, but unfortunately the patch doesn't cleanly apply or work without it. I'm going to get back to this today.
Member

jordanlewis commented Oct 15, 2018

Glad you asked. I think I'm missing a backport. It's unfortunate that this backport required the local exprs branch in the first place. It would be much better to not require that, but unfortunately the patch doesn't cleanly apply or work without it. I'm going to get back to this today.
@asubiotto

This comment has been minimized.

Show comment
Hide comment
@asubiotto

asubiotto Oct 15, 2018

Contributor

How important is EXPLAIN (DISTSQL) support for subqueries in 2.1?

BTW re: EXPLAIN ANALYZE failures, the expected url seems wrong: https://cockroachdb.github.io/distsqlplan/decode.html#eJyckEFLxDAQhe_-ivBOCgHba0DYRRAKS5XdepIess2wBNKmzKSoLPnv0uYg3mSP772ZNx9zxRQdtXYkgflAjV5j5jiQSOTVKgON-4KpNPw0L2m1e40hMsFckXwKBIPOngMdyTrixwoajpL1Yaud2Y-Wv3f2PEDjxYdEbNSuVk_qeX_q7tv3w0HtT6ppuwdocPwUxWSdUWuRJBuCSn4koypBnzXikn5BJNkLwdRZ_x_2SDLHSegv522Hq9xrkLtQ-ZfEhQd64zhsFEW-bnub4UhSSesimqlEuc93PwEAAP__33uBjw==. I'm confused as to why we expect the Result to have stats. This doesn't seem to be the same expected string as on master.
Contributor

asubiotto commented Oct 15, 2018

How important is EXPLAIN (DISTSQL) support for subqueries in 2.1?

BTW re: EXPLAIN ANALYZE failures, the expected url seems wrong: https://cockroachdb.github.io/distsqlplan/decode.html#eJyckEFLxDAQhe_-ivBOCgHba0DYRRAKS5XdepIess2wBNKmzKSoLPnv0uYg3mSP772ZNx9zxRQdtXYkgflAjV5j5jiQSOTVKgON-4KpNPw0L2m1e40hMsFckXwKBIPOngMdyTrixwoajpL1Yaud2Y-Wv3f2PEDjxYdEbNSuVk_qeX_q7tv3w0HtT6ppuwdocPwUxWSdUWuRJBuCSn4koypBnzXikn5BJNkLwdRZ_x_2SDLHSegv522Hq9xrkLtQ-ZfEhQd64zhsFEW-bnub4UhSSesimqlEuc93PwEAAP__33uBjw==. I'm confused as to why we expect the Result to have stats. This doesn't seem to be the same expected string as on master.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment