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.

Sign up for GitHub

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

Jump to bottom

fix: Query fields are sorted by creation #97

Merged
merged 2 commits into from
Jul 18, 2022
Merged

fix: Query fields are sorted by creation #97

merged 2 commits into from
Jul 18, 2022

Conversation

dust1
Copy link
Contributor

@dust1 dust1 commented Jul 13, 2022

Which issue does this PR close?

Closes #60

Rationale for this change

What changes are included in this PR?

This PR modifies the Response data structure in ceresdb/server/src/handlers/sql.rs.

  • ResponseRow: one record with response
  • ResponseRows: type of Vec
  • ResponseColumn: column struct, for keep column type and name
  • Row: Helper type that is convenient for http serialize, this is one map
  • mod http_format: ResponseRow in custom format by Serializer
  • mod row_format: Row in custom format by Serializer

Are there any user-facing changes?

not

How does this change test

@MichaelLeeHZ

This comment was marked as spam.

Sign in to view
@waynexia waynexia self-requested a review July 14, 2022 02:45
waynexia
waynexia reviewed Jul 14, 2022
View reviewed changes
Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

Thanks @dust1 for fixing this! The implementation generally looks good to me. I left some style comments.

And I'm wondering if we can add some assertions on the serialized JSON format in test (if SerializeMap can preserve the ordering, not sure).

server/src/handlers/sql.rs Outdated Show resolved Hide resolved
server/src/handlers/sql.rs Outdated Show resolved Hide resolved
server/src/handlers/sql.rs Outdated Show resolved Hide resolved
waynexia
waynexia approved these changes Jul 18, 2022
View reviewed changes
Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

👍 This looks good to me. Thanks @dust1. I'm going to merge it this afternoon.

@waynexia waynexia merged commit bf6cf18 into apache:main Jul 18, 2022
@dust1 dust1 deleted the issue60 branch July 18, 2022 06:26
@jiacai2050 jiacai2050 mentioned this pull request Jan 4, 2023
Merged
chunshao90 pushed a commit to chunshao90/ceresdb that referenced this pull request May 15, 2023
@dust1
fix: Query fields are sorted by creation (apache#97)
c8d9312 
* fix: Query fields are sorted by creation

* edit: remove sub mod, use impl Serialize for ResponseRows/Row
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@waynexia waynexia waynexia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Query fields are sorted by creation
3 participants
@dust1 @MichaelLeeHZ @waynexia