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.

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

avoid CatalogType::TABLE_MACRO_ENTRY which can cause SIGABRT #5

Merged
merged 6 commits into from
Jun 28, 2024

Conversation

hmeriann
Copy link
Contributor

StatementGenerator::GetDatabaseState() recognises the CatalogType::TABLE_MACRO_ENTRY as a table_functions according to the CatalogSet &DuckSchemaEntry::GetCatalogSet(CatalogType type).

During a discussion with @Tmonster it was decided to avoid CatalogType::TABLE_MACRO_ENTRY in the Statement Generator.

@Tmonster Tmonster self-requested a review June 25, 2024 12:59
auto &table_function_ref = Choose(generator_context->table_functions);
auto &entry = table_function_ref.get().Cast<TableFunctionCatalogEntry>();
auto original_val = generator_context->table_functions.size();
auto random_fun = RandomValue(original_val);
Copy link
Collaborator

Choose a reason for hiding this comment

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

random_val?

auto random_fun = RandomValue(original_val);
auto table_function_ref = &generator_context->table_functions[random_fun];
while (table_function_ref->get().type == CatalogType::TABLE_MACRO_ENTRY) {
table_function_ref = &generator_context->table_functions[RandomValue(original_val)];
Copy link
Collaborator

Choose a reason for hiding this comment

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

can the logic of the while loop change a bit?

1. random_val +=1 % generator_context->table_functions.size();
2. if (random_val == original_val) -> Error
3. table_function_ref = &generator_context->table_functions[random_val]

This way we can prevent an infinite loop

@hmeriann hmeriann requested a review from Tmonster June 25, 2024 13:48
if (random_val == original_val) {
throw InternalException("No table_functions to test.");
}
table_function_ref = &generator_context->table_functions[RandomValue(original_val)];
Copy link
Collaborator

Choose a reason for hiding this comment

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

RandomValue(original_val) is still here, you can just have

table_function_ref = &generator_context->table_functions[random_val]; since random_val was increased by 1

@hmeriann hmeriann requested a review from Tmonster June 27, 2024 09:44
auto table_function_ref = &generator_context->table_functions[random_val];
while (table_function_ref->get().type == CatalogType::TABLE_MACRO_ENTRY) {
random_val += 1 % original_val;
if (random_val == original_val) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry, but realizing this is still wrong.

original_val is set to the size of the generator_context->table_functions vector.
original_val should be the first random value (i.e the RandomValue() returned on line 521.

Then random_val can be increased in the while loop (with the modulo) until it is equal to original_val

Copy link
Collaborator

Choose a reason for hiding this comment

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

Basically it should look like

auto num_table_functions = generator_context->table_functions.size();
auto random_val = RandomValue(original_val);
auto original_val = random_val;
auto table_function_ref = &generator_context->table_functions[random_val];
while (table_function_ref->get().type == CatalogType::TABLE_MACRO_ENTRY) {
    random_val += 1; // I also think the + and modulo needs to be split up, 
                     // otherwise you get random_val = random_val + (1 % num_table_functions)
    random_val = random_val % num_table_functions;
    if (random_val == original_val) {
        ....
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I didn't get all these details earlier. Fixed

@hmeriann hmeriann requested a review from Tmonster June 28, 2024 07:25
Copy link
Collaborator

@Tmonster Tmonster left a comment

Choose a reason for hiding this comment

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

great thanks!

@Tmonster Tmonster merged commit 4a6da30 into duckdb:main Jun 28, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants