Skip to content

Rename count, int, real, and addr to uint64, int64, double, and ip respectively #2864

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

Merged
merged 14 commits into from
Jan 15, 2023

Conversation

dominiklohmann
Copy link
Member

@dominiklohmann dominiklohmann commented Jan 13, 2023

This changes the basic type names of the count, int, real, and addr types to uint64, int64, double, and ip. It also changes the type extractors accordingly.

The change is written in a way that is backwards-compatible: the old type tokens are still recognized and parsed by VAST, and a warning is omitted once per deprecated type token on startup if it is parsed.

This PR does intentionally not change the identifier of the Arrow extension type for the ip type from vast.address to vast.ip, as that is a breaking change that while possible to implement requires a lot of effort. If we want to change that we can always do so in the future, but I intentionally spared myself from it for this PR.

Most of this renaming was created semi-automatically through the help of clangd and :lua vim.lsp.buf.rename(), and some clever git-grepping in combination with quickfix lists and :cdo s/old/new/g. I don't expect this to be thoroughly reviewed, as I don't think this is possible given the number of changes. Please do look at the documentation changes.

@dominiklohmann dominiklohmann added feature New functionality refactoring Restructuring of existing code labels Jan 13, 2023
@dominiklohmann dominiklohmann requested a review from mavam January 13, 2023 21:07
@dominiklohmann dominiklohmann force-pushed the story/sc-40551/rename-basic-types branch 4 times, most recently from 6f16425 to 7100706 Compare January 13, 2023 21:39
@dominiklohmann dominiklohmann force-pushed the story/sc-40551/rename-basic-types branch 3 times, most recently from 4df3c8f to f847244 Compare January 13, 2023 23:14
@dominiklohmann dominiklohmann force-pushed the story/sc-40551/rename-basic-types branch from f847244 to 2b4e8de Compare January 14, 2023 00:34
This change makes it such that `-1 < 0u` correctly returns false rather
true in query evalaution. Queries cannot run into this yet due to the
lack of type coercion, but with the switch from `vast::integer` to
`int64_t` clang (rightfully) started emitting warnings about this.
@dominiklohmann dominiklohmann force-pushed the story/sc-40551/rename-basic-types branch from 2b4e8de to 3042f23 Compare January 14, 2023 03:48
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

Nice 🐒 work.

I think this PR is the right place to also add the corresponding blog post pieces that describe the change.

@dominiklohmann
Copy link
Member Author

I think this PR is the right place to also add the corresponding blog post pieces that describe the change.

See #2865.

@dominiklohmann dominiklohmann merged commit f53b88d into master Jan 15, 2023
@dominiklohmann dominiklohmann deleted the story/sc-40551/rename-basic-types branch January 15, 2023 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality refactoring Restructuring of existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants