exec: check for unset output columnTypes #37571
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @asubiotto and @jordanlewis)
pkg/sql/distsqlrun/column_exec_setup.go, line 34 at r1 (raw file):
"github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/cockroach/pkg/util/tracing" opentracing "github.com/opentracing/opentracing-go"
[nit]: I'm still unsure what our policy is about these "import variables."
pkg/sql/distsqlrun/column_exec_setup.go, line 469 at r1 (raw file):
if columnTypes == nil { return nil, pgerror.Newf(pgerror.CodeDataExceptionError, "output columnTypes unset after planning %T\n", op)
Maybe this should be a panic?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlrun/column_exec_setup.go, line 34 at r1 (raw file):
Previously, yuzefovich wrote…
[nit]: I'm still unsure what our policy is about these "import variables."
cc @jordanlewis. Not sure we have a policy about this. This just means (I think) that the directory is called opentracing-go (to differentiate from all the other language implementations) but the go package itself is opentracing. This is a change that goland puts in for me as it sees that it's unnecessary to use an alias. I would prefer not using an alias if it's not necessary.
pkg/sql/distsqlrun/column_exec_setup.go, line 469 at r1 (raw file):
Previously, yuzefovich wrote…
Maybe this should be a panic?
Maybe, it's a good point. However, they were errors in the projection op before though. I would probably lean towards making this a panic.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlrun/column_exec_setup.go, line 469 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Maybe, it's a good point. However, they were errors in the projection op before though. I would probably lean towards making this a panic.
My understanding is that we need to set columnTypes all the time, so if they are null, then we should panic as a reminder to set them.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlrun/column_exec_setup.go, line 34 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
cc @jordanlewis. Not sure we have a policy about this. This just means (I think) that the directory is called
opentracing-go(to differentiate from all the other language implementations) but the go package itself isopentracing. This is a change that goland puts in for me as it sees that it's unnecessary to use an alias. I would prefer not using an alias if it's not necessary.
Not sure but I don't think it matters.
pkg/sql/distsqlrun/column_exec_setup.go, line 469 at r1 (raw file):
Previously, yuzefovich wrote…
My understanding is that we need to set
columnTypesall the time, so if they are null, then we should panic as a reminder to set them.
I think this should be a pgerror.NewAssertionErrorf. We don't catch panics here - internal errors are the way to go.
Future planning PRs need these to be set, so this commit introduces a check for unset columnTypes that will return an error. Release note: None
There was a problem hiding this comment.
bors r+
Reviewable status:
pkg/sql/distsqlrun/column_exec_setup.go, line 469 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
I think this should be a
pgerror.NewAssertionErrorf. We don't catch panics here - internal errors are the way to go.
Done.
Build failed (retrying...) |
35591: libroach: enable rocksdb WAL recycling r=ajkr a=ajkr This avoids frequent inode writeback during `fdatasync()` from the database's third WAL onwards. It helps on filesystems that preallocate unwritten extents, like XFS and ext4, and also filesystems that don't support `fallocate()`, like ext2 and ext3. Release note: None 37571: exec: check for unset output columnTypes r=asubiotto a=asubiotto Future planning PRs need these to be set, so this commit introduces a check for unset columnTypes that will return an error. Release note: None Co-authored-by: Andrew Kryczka <andrew.kryczka2@gmail.com> Co-authored-by: Alfonso Subiotto Marqués <alfonso@cockroachlabs.com>
Build succeeded |
Future planning PRs need these to be set, so this commit introduces a
check for unset columnTypes that will return an error.
Release note: None