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

Displayer for QueryRowsResult #1138

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Displayer for QueryRowsResult #1138

wants to merge 2 commits into from

Conversation

Michu1596
Copy link

@Michu1596 Michu1596 commented Dec 4, 2024 •
edited
Loading

This PR adds Displayer for QueryRowsResult trying to mimic cqlsh way of displaying tables, but there are some differences:

  • I implement the display for the duration type differently, because I think cqlsh displays this type in an unclear way,for more details look in the comments.
  • When displaying collections (list, map, set, tuple, udt), I use quotes only for text as I think putting other types in quotes might be confusing.

Fixes: #999

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@wprzytula wprzytula changed the title Displayer for QueryRowsResult #999 Displayer for QueryRowsResult Dec 4, 2024
@Michu1596 Michu1596 force-pushed the query_rows_result_displayer branch from c8ad86d to ddf3d5d Compare December 6, 2024 16:20
@Michu1596
Copy link
Author

obraz
screenshot shows results of command SCYLLA_URI="127.0.0.1:9042" cargo run --example displayer

note the difference between the way cqlsh is displaying duration
obraz

@Michu1596 Michu1596 force-pushed the query_rows_result_displayer branch from 3a3f682 to e15634e Compare January 10, 2025 16:57
@Michu1596 Michu1596 marked this pull request as ready for review January 10, 2025 17:00
@Michu1596 Michu1596 force-pushed the query_rows_result_displayer branch from e15634e to e822850 Compare January 13, 2025 13:47
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 13, 2025 •
edited
Loading

cargo semver-checks found no API-breaking changes in this PR.
Checked commit: 475ab0d

@Michu1596 Michu1596 force-pushed the query_rows_result_displayer branch 4 times, most recently from 54ad40d to 7b10b8b Compare January 15, 2025 11:04
wprzytula
wprzytula requested changes Jan 15, 2025
View reviewed changes
Copy link
Collaborator

@wprzytula wprzytula left a comment

Choose a reason for hiding this comment

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

💯 This is a really great job done! In the aspect of usability, I'm very satisfied.

🔧 There are a few items to be addressed, though, mainly about:

  • efficiency: I believe we can get rid of String allocations on the way to display,
  • more idiomatic code,
  • division into modules.

scylla/src/response/query_result.rs Outdated
}
}

/// A utility struct for displaying rows received from the database in a [`QueryRowsResult`].
Copy link
Collaborator

Choose a reason for hiding this comment

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

📌 As I noted elsewhere, we should consider making this work both with QueryRowsResult and with TypedRowStream.

@Michu1596 Michu1596 force-pushed the query_rows_result_displayer branch from 7b10b8b to 7687302 Compare January 15, 2025 11:43
@wprzytula wprzytula added the QoL Quality of Life enhancements label Jan 15, 2025
@wprzytula wprzytula added this to the 1.x.0 milestone Jan 15, 2025
@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Jan 21, 2025
@Michu1596 Michu1596 force-pushed the query_rows_result_displayer branch from 222febc to 549fd82 Compare January 22, 2025 10:26
@github-actions github-actions bot removed the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Jan 22, 2025
wprzytula
wprzytula requested changes Jan 23, 2025
View reviewed changes
scylla/src/response/rows_displayer.rs Outdated
Comment on lines 864 to 861
impl fmt::Display for WrapperDisplay<'_, CqlDate> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
// example of formating date 2021-12-31
let magic_constant = 2055453495; // it is number of days from -5877641-06-23 to -250000-01-01
// around -250000-01-01 is the limit of naive date

Copy link
Collaborator

Choose a reason for hiding this comment

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

❓ Please explain in detail what the "magic constant" is and why it is correct in your opinion.

Copy link
Author

Choose a reason for hiding this comment

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

I believe it is correct because the dates printed by the displayer are the same as those inserted into the database, this constant allows shifting the date calculation to a valid range, thus preventing from_ymd_opt() from returning None. The date -5877641-06-23 is the reference point for CqlDate.

@Michu1596 Michu1596 force-pushed the query_rows_result_displayer branch from 549fd82 to 9b05d0e Compare January 26, 2025 17:12
@Michu1596
Copy link
Author

rebased on main

@Michu1596
Copy link
Author

addressed comments

@wprzytula wprzytula self-requested a review January 26, 2025 21:10
wprzytula
wprzytula reviewed Jan 27, 2025
View reviewed changes
Copy link
Collaborator

@wprzytula wprzytula left a comment

Choose a reason for hiding this comment

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

💯 Looks good!

Could you please squash the 3 latter commits into the respective former 2? So that nothing is introduced in a bad way in one commit only to be fixed in another.

@wprzytula
Copy link
Collaborator

🔧 There are new merge conflicts; when squashing the commits I asked you for, please remember to rebase on main.

@Michu1596 Michu1596 force-pushed the query_rows_result_displayer branch 3 times, most recently from 369b40c to 20d1534 Compare January 29, 2025 08:52
wprzytula
wprzytula previously approved these changes Jan 29, 2025
View reviewed changes
Copy link
Collaborator

@wprzytula wprzytula left a comment

Choose a reason for hiding this comment

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

💯 🎉 Looks good to me! Thank you for the contribution!

@wprzytula wprzytula modified the milestones: 1.x.0, 1.1.0 Mar 7, 2025
@Michu1596 Michu1596 force-pushed the query_rows_result_displayer branch from 20d1534 to cbad25f Compare March 8, 2025 19:45
@Michu1596 Michu1596 force-pushed the query_rows_result_displayer branch from 57d9a2f to 475ab0d Compare March 8, 2025 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wprzytula wprzytula wprzytula left review comments

@muzarski muzarski Awaiting requested review from muzarski

@Lorak-mmk Lorak-mmk Awaiting requested review from Lorak-mmk

At least 1 approving review is required to merge this pull request.

Assignees

@Michu1596 Michu1596

Labels
QoL Quality of Life enhancements
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

Implement Display for QueryResult
2 participants
@Michu1596 @wprzytula