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

environmentd: convert json numbers to strings #21664

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

maddyblue
Copy link
Contributor

This used to happen but regressed at some point.

Fixes MaterializeInc/database-issues#6508

Motivation

  • This PR fixes a recognized bug.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • This PR includes the following user-facing behavior changes:
    • Change the HTTP and WebSocket JSON interfaces of the sql API to format numeric results as strings instead of JSON numbers.

@maddyblue maddyblue requested review from a team as code owners September 8, 2023 08:21
Copy link
Member

@ParkMyCar ParkMyCar left a comment

Choose a reason for hiding this comment

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

This approach works for me, just two thoughts:

  1. What about websockets, it looks like they directly create a SqlResult::Rows without calling this method.
  2. What do you think about making this change as part of the ToJson impl for TypedDatum?

@maddyblue
Copy link
Contributor Author

Ah! SUBSCRIBE over ws didn't use this. Ergh. Yes, putting this into TypedDatum.json is a way better choice. Added code for that.

@maddyblue maddyblue force-pushed the json-string branch 2 times, most recently from e55f9f2 to cced752 Compare September 10, 2023 21:33
This used to happen but regressed at some point.

Fixes #21659
@maddyblue maddyblue merged commit a48ab5a into MaterializeInc:main Sep 11, 2023
@maddyblue maddyblue deleted the json-string branch September 11, 2023 20:00
@antiguru
Copy link
Member

This breaks the memory visualization because it now compares integers with strings.

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