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

chore: add better regclass and oid cast support #2795

Merged
merged 5 commits into from
Mar 20, 2024

Conversation

universalmind303
Copy link
Contributor

partially addresses #2746

Copy link
Contributor

@tychoish tychoish left a comment

Choose a reason for hiding this comment

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

few questions, seems reasonable.

(shouldn't this be a fix not a chore?)

@@ -0,0 +1,30 @@

statement error
Copy link
Contributor

Choose a reason for hiding this comment

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

should we put the error message here?

Comment on lines +23 to +26
statement error Casting expressions to oid unsupported
select cast(1 as oid);

statement error Casting expressions to oid unsupported
Copy link
Contributor

Choose a reason for hiding this comment

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

should this error message be phrased as "only strings can be cast to OIDs"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error message was copied from the regclass error. I can change it, but used this to remain consistent with the existing implementation.

select cast('my_table' as OID);


# We don't support expressions other than strings right now.
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a point to supporting more ever/eventually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk, most of this was copied from the regclass stuff, so i didn't want to change any more than absolutely necessary to implement the new functionality

@universalmind303
Copy link
Contributor Author

(shouldn't this be a fix not a chore?)

IDK, aruguably it could also be a feature...

@tychoish
Copy link
Contributor

The existing regclass error is only used once, and is ambiguous from the user's perspective (and ours, when debugging) if they ever run into it, particularly now that we (presumably) support integer and string casts. If it's easy enough to provide a bit more context, it seems worthwhile.

I don't see any particular reason to treat our error messages as a stable API, unless I'm missing something or this is a standard/copied error message.

@universalmind303 universalmind303 enabled auto-merge (squash) March 20, 2024 12:40
@universalmind303 universalmind303 merged commit f4c541b into main Mar 20, 2024
25 checks passed
@universalmind303 universalmind303 deleted the universalmind303/2746 branch March 20, 2024 12:52
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.

3 participants