-
Notifications
You must be signed in to change notification settings - Fork 6
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
Replace hardcoded percentages with values from a file #27
base: main
Are you sure you want to change the base?
Conversation
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.
Looks good! I don't think the RandomsConfig needs to be a friend class, instead you can make the RandomConfig a member of the StatementGenerator. Ie...
class StatementGenerator {
public:
RandomsConfig percentage_selector;
}
…rom the config file
00d7930
to
e08843b
Compare
…rkflow directory of the source repo from the reusable_workflows directory
…s and that doesn't make ParseJSONMap parse the string with the new lines
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.
Maybe the RandomNumsConfig
could be an unorderdered_map<percentages_enum, idx_t>
?
You will have to create an Enum (see duckdb/common/enums/logical_operator_type.hpp
for an example).
In the future it will make it easier to manage the percentages, add new ones etc.
…h the wrong json structure or contains an empty {}
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.
Looks much better.
looks like you don't use the file handle though.
Now you can start adding more percentages as well if you want. Feel free to create a GitHub actions file that runs the fuzzer with a config file and one without
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.
Looks good. Just the one comment.
also, I can imagine you may end up adding many ENUMS
for each case. You can also just create enums like RandomPercentagesEnum::DEFAULT_XX_PERCENT
so that your enum library doesn't get insanely big. Eventually the enums in the statement generation code can get their own enums as we continue to work on and expand this functionality
src/include/random_nums_config.hpp
Outdated
|
||
}; | ||
|
||
unordered_map<RandomPercentagesEnum, idx_t> GetDefaultConfig(); |
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.
Can the RandomNumsConfig
be a class? That should make things easier to maintain in my opinion
This PR will replace hardcoded percentages from the fuzzer with values defined in a file, as it mentioned in the #23