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

Feature Request: Move most VARBINARY columns to VARCHAR in examples #15997

Open
1 of 3 tasks
mattlord opened this issue May 22, 2024 · 12 comments · May be fixed by #17318
Open
1 of 3 tasks

Feature Request: Move most VARBINARY columns to VARCHAR in examples #15997

mattlord opened this issue May 22, 2024 · 12 comments · May be fixed by #17318
Labels
Component: Examples good first issue Good issue for new contributors Type: Feature

Comments

@mattlord
Copy link
Contributor

mattlord commented May 22, 2024 •
edited
Loading

Feature Description

The Vitess local examples (source) are a common first touch experience for users. These users are regularly confused or curious as to why many of the columns in the examples show up as HEX in the query results. The reason for this is that we use VARBINARY types throughout — in some rare cases like keyspace_id it makes sense, but most like customer.email are pure text. This makes understanding what's happening and quickly confirming at-a-glance results as expected more difficult and poses a minor hurdle when it's already quite a lot to get started with Vitess.

We should move most of our VARBINARY usage under ./examples to VARCHAR. This offers a more intuitive/expected experience while also more closely matching what users will be using for similar types of data.

Note: if any of those columns are used in Vindexes then we may also need to change the type used.

Tasks:

  • Change non-binary column types from VARBINARY to VARCHAR
  • Change the vindex type used for those columns in related vschema definitions from hash to xxhash
  • Get all of the example CI tests passing again

Use Case(s)

An easier on-ramp to Vitess.
@mattlord mattlord mentioned this issue May 22, 2024
Merged
5 tasks
@EraKin575
Copy link

I would like to work on this issue

@lata-11
Copy link

lata-11 commented Jul 8, 2024

Hey! I would be happy to raise a PR for this.

@mattlord
Copy link
Contributor Author

mattlord commented Jul 8, 2024

Anyone is welcome to open a PR. I don't want to assign it to anyone before there's a PR because history has shown that the assignee often doesn't end up opening a PR, nor do they unassign themselves, so others think someone is working on it when in reality nobody is. Then maintainers have to come back and unassign it after N months and around we go. 🙂

@kmr-rohit kmr-rohit mentioned this issue Jul 22, 2024
Closed
5 tasks
@anshikavashistha
Copy link
Contributor

@mattlord May I work on this issue if this is still open ?

@mattlord
Copy link
Contributor Author

@mattlord May I work on this issue if this is still open ?

@anshikavashistha sure, nobody else is working on this AFAIK. If you open a preliminary PR then I'll assign it to you. Assigning things like this to people has historically caused more work for maintainers and potentially caused others not to work on it when in the end nothing happened. Thanks!

@anshikavashistha
Copy link
Contributor

@mattlord May I work on this issue if this is still open ?

@anshikavashistha sure, nobody else is working on this AFAIK. If you open a preliminary PR then I'll assign it to you. Assigning things like this to people has historically caused more work for maintainers and potentially caused others not to work on it when in the end nothing happened. Thanks!

Sure @mattlord
Thank you

@anshikavashistha
Copy link
Contributor

image
@mattlord @deepthi ,I have to change VARBINARY columns to VARCHAR where appropriate or there might not be no need to move VARBINARY columns as per this relevant PR

@deepthi
Copy link
Member

deepthi commented Sep 3, 2024 •
edited
Loading

The question is more for @mattlord. Given the change you made to the alias, many of the columns in the examples show up as HEX in the query results does not happen any more. Do we still want to change the data types?
As it turns out, none of the VARBINARY columns are being used as sharding columns, so it's not as if we are missing out on providing an example of how to shard using VARCHAR columns.

@mattlord
Copy link
Contributor Author

mattlord commented Sep 4, 2024

I think it's still worth doing as the type usage then aligns with what users would do, we move to xxhash, etc.

@deepthi
Copy link
Member

deepthi commented Sep 4, 2024 •
edited
Loading

In that case, can you edit the issue description with the task list? Including moving from hash -> xxhash.

@anshikavashistha
Copy link
Contributor

@mattlord Is there any more relevant info that needs to be mentioned in the issue description?

@mattlord
Copy link
Contributor Author

@anshikavashistha I don't think so. I did add a task list — hopefully that makes sense.

@arthmis arthmis linked a pull request Dec 2, 2024 that will close this issue
Open
5 tasks
@Hana-bii Hana-bii mentioned this issue Dec 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Component: Examples good first issue Good issue for new contributors Type: Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Feature Request: Change varbinary to varchar in schemas used in examples folder arthmis/vitess
5 participants
@deepthi @mattlord @anshikavashistha @EraKin575 @lata-11