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: added inet function to support conversion to inet type #83668

Merged

Conversation

surahman
Copy link
Contributor

@surahman surahman commented Jun 30, 2022

Release note (sql change):
The inet function has been added to support the conversion of a supplied type to that of the inet
type family. If the conversion fails a SQL error will be output.

Closes #82052.

@surahman surahman requested a review from a team June 30, 2022 17:50
@blathers-crl
Copy link

blathers-crl bot commented Jun 30, 2022

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added the O-community Originated from the community label Jun 30, 2022
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@surahman
Copy link
Contributor Author

surahman commented Jun 30, 2022

  • I am doubtful of whether the SQL parser needs to be updated to handle this or if there is some automated generation process. Some of the builtins do not provide any additions to the SQL parser.
  • The function currently returns an error if the conversion fails. I was unable to find the documentation for the inet function in PostreSQL. MySQL handles a failed conversion by returning a NULL. How should this case be handled?
  • I have selected the input as type.Any. The issue did not specify a specific conversion type (sample contains string) so I went the route of trying to convert whatever type is provided. How should input type be defined?
  • The user documentation in docs/generated/sql also needs to be autogenerated.

@surahman surahman force-pushed the CRDB-16454-inet_conversion_builtin branch from 680620c to ffcb34f Compare June 30, 2022 20:44
@blathers-crl
Copy link

blathers-crl bot commented Jun 30, 2022

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@surahman surahman force-pushed the CRDB-16454-inet_conversion_builtin branch from ffcb34f to a073cdd Compare June 30, 2022 21:03
@blathers-crl
Copy link

blathers-crl bot commented Jun 30, 2022

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

  • We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
  • When CI has completed, please ensure no errors have appeared.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@surahman surahman force-pushed the CRDB-16454-inet_conversion_builtin branch 4 times, most recently from bf41b82 to 970bc9c Compare July 1, 2022 00:12
@surahman
Copy link
Contributor Author

surahman commented Jul 1, 2022

Not sure about the benchmark failures there are no details in the CI logs. I think they might be spurious. Everything passes locally. This is failing on exactly the same tests which were run in the same time window.

@surahman surahman force-pushed the CRDB-16454-inet_conversion_builtin branch 2 times, most recently from 1a68491 to ddc82fb Compare July 5, 2022 17:51
@surahman surahman force-pushed the CRDB-16454-inet_conversion_builtin branch 2 times, most recently from 31d9630 to 0f658bb Compare July 11, 2022 22:11
@surahman surahman requested review from a team as code owners July 11, 2022 22:11
@surahman surahman force-pushed the CRDB-16454-inet_conversion_builtin branch from 0f658bb to c719bae Compare July 11, 2022 22:13
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

thank you for your contribution!

i left a few small comments, but this looks good

you will need to run ./dev generate docs or make buildshort to make sure the generated docs are up to date

please address the comments, and amend your original commit with the changes. thanks!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @surahman)


-- commits line 5 at r1:
nit: please add newlines to the message so it's at most 100 characters wide


pkg/sql/sem/builtins/builtins.go line 1051 at r1 (raw file):

	"inet": makeBuiltin(defProps(),
		tree.Overload{
			Types:      tree.ArgTypes{{"val", types.Any}},

to match postgres, we should make this only accept types.String. (PG actually accepts type cstring here, but CRDB doesn't have that)


pkg/sql/sem/builtins/builtins.go line 1057 at r1 (raw file):

				if err != nil {
					return nil, pgerror.Newf(
						pgcode.InvalidParameterValue, "could not convert to INet: %s", args[0])

to match postgres, let's use pgcode.InvalidTextRepresentation

also, instead of pgerror.Newf, please use the original error

return nil, pgerror.WithCandidateCode(err, pgcode.InvalidTextRepresentation)

pkg/sql/sem/builtins/builtins.go line 1061 at r1 (raw file):

				return inet, nil
			},
			Info:       "If possible, converts input to that of type INet.",

please use "inet" instead of "INet"

Release note (sql change):
The inet function has been added to support the conversion of a supplied type to that of the inet
type family. If the conversion fails a SQL error will be output.
@surahman surahman force-pushed the CRDB-16454-inet_conversion_builtin branch from c719bae to a63ecf8 Compare July 15, 2022 15:50
@surahman
Copy link
Contributor Author

Thank you @rafiss

@surahman
Copy link
Contributor Author

Bazel CI failure appears to be unrelated.

[17:02:04][Run unit tests] === RUN   TestTenantLogic/3node-tenant/grant_on_all_sequences_in_schema
[17:02:04][Run unit tests] [17:02:02] setting distsql_workmem='34458B';
[17:02:04][Run unit tests] [17:02:02] setting distsql_workmem='34458B';
[17:02:04][Run unit tests] [17:02:03] rng seed: 1456840546540403413
[17:02:04][Run unit tests] 
[17:02:04][Run unit tests] -- Test timed out at 2022-07-15 17:02:03 UTC --

grant_on_all_sequences_in_schema

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

thanks for your work! this looks good to me

bors r+

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @surahman)

@surahman
Copy link
Contributor Author

Thank you @rafiss

@craig
Copy link
Contributor

craig bot commented Jul 19, 2022

Build succeeded:

@craig craig bot merged commit da0a1b8 into cockroachdb:master Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-community Originated from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support inet function postgres
3 participants