Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Advanced partitioning customizations (COMPOSER-2355) #926
base: main
Are you sure you want to change the base?
Advanced partitioning customizations (COMPOSER-2355) #926
Changes from 16 commits
f39037a
805ece4
68fb75a
548e871
ae2d48b
e0ba877
8b5653d
80ce76d
766911f
7e9ca23
f88e6e2
397efb0
9b75803
4a689db
51c44a5
986b64c
d7b9dd4
24b83ca
d844a8a
4e7a7ee
2a0bc1b
9a5f8dd
92f089b
5da5eae
bcfb63a
d50bf94
84d94e0
d9fe555
541f95f
c7c3486
837ce9b
9451486
01b0ab7
5bf3416
f6fd14a
0266934
6b1f037
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we don't test this right now? It's fine if we don't, I would love to have a test for this and also one that checks that the json strings are what we expect but I don't want this PR to become bigger, I can offer to do it as a followup :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to existing tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about making this a
string
alias, and use descriptive values? Should make debugging a tad bit simpler. Are there downsides (apart from slightly worse performance)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's not much downside other than making it "easy" to use strings in place of the actual value sometimes, which can make it hard to change or track. Having the enum backed by a uint says, a bit more clearly, "the underlying value doesn't matter", which I find encourages better use of the enum.
But I can go either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw, I had the same thought and tried it out and it was the same about the amount of code/complexity, golang just does not have a good way of doing this it feels so I did not comment (maybe I should have).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any strong feelings either way? Should I go with strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't have strong feelings. :)