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

sql/parser: Move DatumToOid map to Oid() method on parser.Type #12642

Merged

Conversation

nvanbenschoten
Copy link
Member

@nvanbenschoten nvanbenschoten commented Jan 3, 2017

This initial change removes a map from parser.Type to oid.Oid and
replaces it with a method on parser.Type. This allows us to avoid a
previous issue with reflection, and allows us to simplify some code that
was performing the same mapping in pgwire.

This is the first commit of #12641, but is a nice cleanup in isolation as well.


This change is Reviewable

@knz
Copy link
Contributor

knz commented Jan 3, 2017

:lgtm: modulo the two comments


Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


pkg/sql/parser/type.go, line 410 at r1 (raw file):

	case TypeString:
		return oid.T__text
	case TypeAny:

See my comment about this in #12641 (comment)


pkg/sql/parser/type.go, line 449 at r1 (raw file):

// Oid implements the Type interface.
func (TTable) Oid() oid.Oid {
	panic("TODO(knz) unknown Oid for TTable")

T_anyelement See my comment about this in #12641 (comment)


Comments from Reviewable

@jordanlewis
Copy link
Member

Nice!


Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


pkg/sql/parser/type.go, line 126 at r1 (raw file):

	oid.T_text:        TypeString,
	oid.T__text:       TypeStringArray,
	oid.T__int2:       TypeIntArray,

Not sure if it matters but you're missing an entry for oid.T_unknown here. I see that it's not a unique mapping - perhaps that's an issue?

Also missing an entry for T_record.


Comments from Reviewable

This initial change removes a map from `parser.Type` to `oid.Oid` and
replaces it with a method on `parser.Type`. This allows us to avoid a
previous issue with reflection, and allows us to simplify some code that
was performing the same mapping in pgwire.
@nvanbenschoten
Copy link
Member Author

TFTRs!


Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


pkg/sql/parser/type.go, line 126 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Not sure if it matters but you're missing an entry for oid.T_unknown here. I see that it's not a unique mapping - perhaps that's an issue?

Also missing an entry for T_record.

Yeah I'm not quite sure that it makes sense to add an entry for oid.T_unknown, based on how we use this mapping. For now, I'll leave it out and wait to see if it's ever needed.

Good catch on the T_record. Done.


pkg/sql/parser/type.go, line 410 at r1 (raw file):

Previously, knz (kena) wrote…

See my comment about this in #12641 (comment)

Done.


pkg/sql/parser/type.go, line 449 at r1 (raw file):

Previously, knz (kena) wrote…

T_anyelement See my comment about this in #12641 (comment)

Done.


Comments from Reviewable

@nvanbenschoten nvanbenschoten merged commit 2f2869c into cockroachdb:master Jan 10, 2017
@nvanbenschoten nvanbenschoten deleted the nvanbenschoten/oidMethod branch January 10, 2017 08:04
@knz
Copy link
Contributor

knz commented Jan 10, 2017

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants