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/pgwire: change formatting of floating point infinity values #64760

Merged
merged 1 commit into from
May 11, 2021

Conversation

iAziz786
Copy link
Contributor

@iAziz786 iAziz786 commented May 6, 2021

Fixes #62601

Postgres drivers such as JDBC rely on infinite values to be
formatted as full words in order to be able to parse the
value.

Release note (sql change): Floating point infinity values are now
formatted as Infinity (or -Infinity if negative). This is for
compatibility with Postgres.

@cockroach-teamcity
Copy link
Member

cockroach-teamcity commented May 6, 2021

CLA assistant check
All committers have signed the CLA.

@blathers-crl
Copy link

blathers-crl bot commented May 6, 2021

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

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

  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

I have added a few people who may be able to assist in reviewing:

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

@blathers-crl blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels May 6, 2021
@blathers-crl blathers-crl bot requested a review from rafiss May 6, 2021 02:41
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@iAziz786
Copy link
Contributor Author

iAziz786 commented May 6, 2021

@rafiss I ran the test cases but for some reason, they were not hitting func (d *DFloat) Format. When I add fmt.Println into that function I only saw one log. Anything am I missing?

@rafiss
Copy link
Collaborator

rafiss commented May 10, 2021

@iAziz786 sorry for misdirecting you! it looks like the change to func (d *DFloat) Format is not necessary.

instead, this change needs to be done in pkg/sql/pgwire/types.go writeTextDatum in the DFloat case.

@iAziz786 iAziz786 force-pushed the improve-inf-support branch from e33be7e to 12b9281 Compare May 11, 2021 01:43
@blathers-crl
Copy link

blathers-crl bot commented May 11, 2021

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.

@iAziz786 iAziz786 force-pushed the improve-inf-support branch from 12b9281 to c649275 Compare May 11, 2021 01:44
@iAziz786
Copy link
Contributor Author

iAziz786 commented May 11, 2021

@rafiss made the suggested changes, and make test PKG=./pkg/sql/pgwire TESTS=TestEncodings is passing as well. Should I move this PR from draft to ready for review?

There is something strange though when I run the command mentioned in the #62601 I'm getting the same result as before.

image

@rafiss
Copy link
Collaborator

rafiss commented May 11, 2021

That's interesting -- it looks like it's because the CLI has extra parse/format logic so that it displays the value as a golang infinity. if you connect with the psql tool (e.g. psql -h localhost -p 26257 -U root -w -d defaultdb), you should see your change in effect

this likely also explains why our other SQL tests are not affected by your change.

yes, please make this ready to review. before that, you will need to update the commit message (and PR message) to have a proper release note. See https://wiki.crdb.io/wiki/spaces/CRDB/pages/186548364/Release+notes

also, the title of the commit message should be adjusted. cockroachdb already supports infinity values -- this is just changing the formatting. see https://wiki.crdb.io/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages

so a full commit message could be:

sql/pgwire: change formatting of floating point infinity values

Postgres drivers such as JDBC rely on infinite values to be
formatted as full words in order to be able to parse the
value.

Release note (sql change): Floating point infinity values are now
formatted as `Infinity` (or `-Infinity` if negative). This is for
compatibility with Postgres

Thanks for your work here! This is almost ready to go.

Postgres drivers such as JDBC rely on infinite values to be
formatted as full words in order to be able to parse the
value.

Release note (sql change): Floating point infinity values are now
formatted as `Infinity` (or `-Infinity` if negative). This is for
compatibility with Postgres
@iAziz786 iAziz786 force-pushed the improve-inf-support branch from c649275 to a4a8e23 Compare May 11, 2021 12:59
@iAziz786 iAziz786 changed the title WIP: support floating point infinity value sql/pgwire: change formatting of floating point infinity values May 11, 2021
@iAziz786
Copy link
Contributor Author

Yes, it's working as you have mentioned. 💯

image

Your version of the commit message is a lot better than what I could have written so I used the same.

@iAziz786 iAziz786 marked this pull request as ready for review May 11, 2021 13:00
@iAziz786 iAziz786 requested a review from a team May 11, 2021 13:00
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!

bors r+

@craig
Copy link
Contributor

craig bot commented May 11, 2021

Build succeeded:

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 X-blathers-triaged blathers was able to find an owner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot read floating point infinity values from JDBC
3 participants