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

sql: ensure VALUES is planned locally when required #32494

Merged
merged 1 commit into from Nov 22, 2018

Conversation

Projects
None yet
3 participants
@jordanlewis
Copy link
Member

jordanlewis commented Nov 19, 2018

Previously, a VALUES node that contained un-distributable scalar
expressions like subqueries or special OID datatypes would be planned
locally, but still be serialized to a DistSQL processor spec protobuf.
This causes incorrect results in some cases.

Now, the physical planner correctly wraps the valuesNode in a
localPlanNode when constrained to the gateway node to avoid the
incorrectness.

Fixes #32422.

Release note (bug fix): prevent VALUES clauses from returning incorrect
results for certain special OID values.

@jordanlewis jordanlewis requested a review from solongordon Nov 19, 2018

@jordanlewis jordanlewis requested review from cockroachdb/distsql-prs as code owners Nov 19, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Nov 19, 2018

This change is Reviewable

@jordanlewis jordanlewis force-pushed the jordanlewis:fix-values-local-planning branch from 8be3619 to d365299 Nov 20, 2018

@jordanlewis jordanlewis requested a review from cockroachdb/sql-opt-prs as a code owner Nov 20, 2018

@jordanlewis jordanlewis force-pushed the jordanlewis:fix-values-local-planning branch 3 times, most recently from e948bab to 8c56b2e Nov 20, 2018

@jordanlewis jordanlewis requested a review from cockroachdb/sql-execution-prs as a code owner Nov 21, 2018

@jordanlewis jordanlewis requested a review from asubiotto Nov 21, 2018

@asubiotto
Copy link
Contributor

asubiotto left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/distsql_physical_planner.go, line 2379 at r1 (raw file):

		// want to be distributing, like populating values from a virtual table. So,
		// we wrap the plan instead.
		if !n.specifiedInQuery || planCtx.isLocal || planCtx.noEvalSubqueries {

Will you be wrapping a values node in cases that don't need to be wrapped? Might also be worth updating the comment.

@jordanlewis jordanlewis force-pushed the jordanlewis:fix-values-local-planning branch from 8c56b2e to 8c85331 Nov 21, 2018

@jordanlewis

This comment has been minimized.

Copy link
Member

jordanlewis commented Nov 21, 2018

Updated the comment. Yes, sometimes valuesNodes will be wrapped when it's not strictly necessary. But, just like with the local Expr serialization optimization, it's more efficient to just wrap these nodes anyway, rather than evaluating, serializing and unserializing the data for each datum within the clause.

sql: ensure VALUES is planned locally when reqd
Previously, a VALUES node that contained un-distributable scalar
expressions like subqueries or special OID datatypes would be planned
locally, but still be serialized to a DistSQL processor spec protobuf.
This causes incorrect results in some cases.

Now, the physical planner correctly wraps the valuesNode in a
localPlanNode when constrained to the gateway node to avoid the
incorrectness.

Release note (bug fix): prevent VALUES clauses from returning incorrect
results for certain special OID values.

@jordanlewis jordanlewis force-pushed the jordanlewis:fix-values-local-planning branch from 8c85331 to 1cc5a5a Nov 21, 2018

@jordanlewis

This comment has been minimized.

Copy link
Member

jordanlewis commented Nov 22, 2018

bors r+

craig bot pushed a commit that referenced this pull request Nov 22, 2018

Merge #32494
32494: sql: ensure VALUES is planned locally when required r=jordanlewis a=jordanlewis

Previously, a VALUES node that contained un-distributable scalar
expressions like subqueries or special OID datatypes would be planned
locally, but still be serialized to a DistSQL processor spec protobuf.
This causes incorrect results in some cases.

Now, the physical planner correctly wraps the valuesNode in a
localPlanNode when constrained to the gateway node to avoid the
incorrectness.

Fixes #32422.

Release note (bug fix): prevent VALUES clauses from returning incorrect
results for certain special OID values.

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
@craig

This comment has been minimized.

Copy link

craig bot commented Nov 22, 2018

Build succeeded

@craig craig bot merged commit 1cc5a5a into cockroachdb:master Nov 22, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@jordanlewis jordanlewis deleted the jordanlewis:fix-values-local-planning branch Nov 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment