Skip to content

Improve ClickHouse data type mapping documentation#13578

Merged
ebyhr merged 1 commit intotrinodb:masterfrom
tlblessing:tb/doc-4123
Aug 17, 2022
Merged

Improve ClickHouse data type mapping documentation#13578
ebyhr merged 1 commit intotrinodb:masterfrom
tlblessing:tb/doc-4123

Conversation

@tlblessing
Copy link
Member

@tlblessing tlblessing commented Aug 9, 2022

Description

Add table for reverse mapping direction. Clean up table formatting.

image

Is this change a fix, improvement, new feature, refactoring, or other?

Improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Connector documentation

How would you describe this change to a non-technical end user or system administrator?

Added a table of the data types from Trino back to ClickHouse. Reformatted table for better wrapping and consistency.

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Aug 9, 2022
@github-actions github-actions bot added the docs label Aug 9, 2022
@tlblessing tlblessing requested review from colebow, ebyhr and jhlodin August 9, 2022 20:05
@tlblessing tlblessing marked this pull request as ready for review August 9, 2022 20:05
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

There's an existing PR #13259. Could you coordinate with @tangjiangling ?

@tlblessing
Copy link
Member Author

There's an existing PR #13259. Could you coordinate with @tangjiangling ?

Sure, I could apply your review comments to my PR. Is that okay with you @tangjiangling ?

cc: @jhlodin

@tangjiangling
Copy link
Member

Is that okay with you @tangjiangling ?

Sure.

Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

I would recommend improving the commit titles to clarify this is document change. It helps us when reading commit history.

  • Add header for ClickHouse type mapping documentation
  • Document write mapping and reformat table in ClickHouse
  • Fix typo in ClickHouse documentation

@ebyhr ebyhr changed the title ClickHouse data type mapping Improve ClickHouse data type mapping documentation Aug 10, 2022
@colebow
Copy link
Member

colebow commented Aug 12, 2022

You're going to need to squash these later commits down into the proper original commits. Trino has a strict policy on commits being atomic, sensible units of change. Code review shouldn't result in extra commits unless the review has prompted you to make a new, independent change which deserves to be separated from the prior work. Trino is similarly strict with commit messages, and we try to follow this guide: https://cbea.ms/git-commit/

When you get feedback or suggestions, you should amend your edits into the commit where you made the change.

My workflow for this is usually:

Make my changes, stage my changes
git log --oneline -3 (or whatever number you need)
Copy the hash of the commit I'm editing
git commit --fixup <hash>
git rebase -i HEAD~3 --autosquash

And that should get all changes into the original commit as appropriate. There's several different ways to accomplish the same thing in Git, but that strategy works best for me.

If you've already made the edit commit like you have here, you can do a rebase, re-order the unnecessary commits to follow the ones they're editing, and change them from pick to fixup to merge them into the proper commits. Rebasing is also the easiest way to rename those original commits.

@mosabua
Copy link
Member

mosabua commented Aug 15, 2022

@tlblessing and @colebow please work on the git-fu together as necessary.

@tlblessing
Copy link
Member Author

@tlblessing and @colebow please work on the git-fu together as necessary.

Will do. Cole is OOO until mid next week.

@tlblessing tlblessing force-pushed the tb/doc-4123 branch 2 times, most recently from c40e96c to d563ed5 Compare August 16, 2022 17:17
@tlblessing
Copy link
Member Author

tlblessing commented Aug 16, 2022

I assume that these types should also be all uppercase @ebyhr ? Except for Float64 etc. ?

Co-Authored-By: Yuya Ebihara <ebyhry@gmail.com>
@ebyhr ebyhr merged commit d1ee6b2 into trinodb:master Aug 17, 2022
@ebyhr
Copy link
Member

ebyhr commented Aug 17, 2022

Merged, thanks!

@github-actions github-actions bot added this to the 393 milestone Aug 17, 2022
@tlblessing tlblessing deleted the tb/doc-4123 branch August 17, 2022 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants