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: Permit placeholders for OID values #14255

Conversation

nvanbenschoten
Copy link
Member

@nvanbenschoten nvanbenschoten commented Mar 19, 2017

Part of #14245.

Previously the tOID type claimed that it was ambiguous through its
IsAmbiguous method. This meant that a placeholderTypeAmbiguityError
would be thrown when issuing queries like SELECT 1::oid = $1. This was
incorrect, as an OID type is fully defined.

This change fixes this and also improves the tOid type slightly. It
gets rid of its redundant typeName field. Additionally, it improves
the type's Equivalent and FamilyEqual methods, which would have
given us issues if one of the "reg oid" variants (typeRegClass,
typeRegProc, etc.) ever entered our type system.


This change is Reviewable

@tamird
Copy link
Contributor

tamird commented Mar 19, 2017

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


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

		return true
	}
	_, ok := UnwrapType(other).(tOid)

this can delegate to FamilyEqual, no?


Comments from Reviewable

@jordanlewis
Copy link
Member

:lgtm: Thanks! I was working on debugging the problem in #14245 this weekend as well, and I also noticed and corrected (but didn't commit) the isAmbiguous implementation. Unfortunately, this doesn't fix the problem I noticed, despite these tests passing. I'll put more details in #14245, but for now I think we'll need to remove the Fixes statement from this PR.


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


Comments from Reviewable

Part of cockroachdb#14245.

Previously the `tOID` type claimed that it was ambiguous through its
`IsAmbiguous` method. This meant that a `placeholderTypeAmbiguityError`
would be thrown when issuing queries like `SELECT 1::oid = $1`. This was
incorrect, as an OID type is fully defined.

This change fixes this, and also improves the `tOid` type slightly. It
gets rid of its redundant `typeName` field. Additionally, it improves
the type's `Equivalent` and `FamilyEqual` methods, which would have
given us issues if one of the "reg oid" variants (`typeRegClass`,
`typeRegProc`, etc.) ever entered our type system.
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/placeholderTypes branch from 6177420 to 9208b50 Compare March 19, 2017 23:49
@nvanbenschoten
Copy link
Member Author

TFTRs! Removed the "Fixes" wording from the commit message.


Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.


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

Previously, tamird (Tamir Duberstein) wrote…

this can delegate to FamilyEqual, no?

Done.


Comments from Reviewable

@tamird
Copy link
Contributor

tamird commented Mar 20, 2017

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@nvanbenschoten nvanbenschoten merged commit 52ec241 into cockroachdb:master Mar 20, 2017
@nvanbenschoten nvanbenschoten deleted the nvanbenschoten/placeholderTypes branch March 20, 2017 00:20
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