Skip to content
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

colflow: error out when phys types mismatch when setting up a flow #44930

Merged
merged 1 commit into from
Feb 11, 2020

Conversation

yuzefovich
Copy link
Member

This commit adds a check that the physical types coming out of the last
columnar operator into the materializer are the same as the materializer
expects. This check is added to SupportsVectorized check, and in case of
types mismatch, we will fallback to the row-by-row engine.

The underlying cause for this particular reproduction is that our
"logical" types system assumes that ints have width 8, which is not
always the case. But trying to fix it is beyond the scope of this PR.

Fixes: #44904.

Release note (bug fix): Previously, CockroachDB could return an internal
error on the queries that return INT columns when the default int size
has been changed, and now this has been fixed.

@yuzefovich yuzefovich requested review from asubiotto and a team February 10, 2020 19:28
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member Author

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto)


pkg/sql/distsql_running.go, line 176 at r1 (raw file):

						},
						NodeID: -1,
					}, spec.Processors, fuseOpt, recv,

I wonder whether we should be on the safe side and pass it a copy of recv?

Copy link
Contributor

@asubiotto asubiotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: minus a nit about how the conditional is formulated

Reviewed 5 of 6 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)


pkg/sql/distsql_running.go, line 176 at r1 (raw file):

Previously, yuzefovich wrote…

I wonder whether we should be on the safe side and pass it a copy of recv?

I think it's fine.


pkg/sql/colflow/vectorized_flow.go, line 939 at r2 (raw file):

) (leaves []execinfra.OpNode, err error) {
	var syncFlowConsumer execinfra.RowReceiver
	if output != nil {
if output == nil {
    output = &execinfra.RowChannel{}
}

And then use output instead of syncFlowConsumer

This commit adds a check that the physical types coming out of the last
columnar operator into the materializer are the same as the materializer
expects. This check is added to SupportsVectorized check, and in case of
types mismatch, we will fallback to the row-by-row engine.

The underlying cause for this particular reproduction is that our
"logical" types system assumes that ints have width 8, which is not
always the case. But trying to fix it is beyond the scope of this PR.

Release note (bug fix): Previously, CockroachDB could return an internal
error on the queries that return INT columns when the default int size
has been changed, and now this has been fixed.
Copy link
Member Author

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto)


pkg/sql/distsql_running.go, line 176 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I think it's fine.

Ok, sgtm.


pkg/sql/colflow/vectorized_flow.go, line 939 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
if output == nil {
    output = &execinfra.RowChannel{}
}

And then use output instead of syncFlowConsumer

Good point, done.

craig bot pushed a commit that referenced this pull request Feb 11, 2020
44930: colflow: error out when phys types mismatch when setting up a flow r=yuzefovich a=yuzefovich

This commit adds a check that the physical types coming out of the last
columnar operator into the materializer are the same as the materializer
expects. This check is added to SupportsVectorized check, and in case of
types mismatch, we will fallback to the row-by-row engine.

The underlying cause for this particular reproduction is that our
"logical" types system assumes that ints have width 8, which is not
always the case. But trying to fix it is beyond the scope of this PR.

Fixes: #44904.

Release note (bug fix): Previously, CockroachDB could return an internal
error on the queries that return INT columns when the default int size
has been changed, and now this has been fixed.

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Feb 11, 2020

Build succeeded

@craig craig bot merged commit 11b6d51 into cockroachdb:master Feb 11, 2020
@yuzefovich yuzefovich deleted the materializer-types branch February 11, 2020 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal error when setting VECTORIZE = experimental_on and DEFAULT_INT_SIZE = 4
3 participants