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

Enable verification - add ORDER BY * #17

Merged
merged 21 commits into from
Aug 7, 2024

Conversation

hmeriann
Copy link
Contributor

This PR makes StatementGenerator generate ORDER BY *.
It is the first step to fuzz with enable_verification.

Connected issue

…TILE function check to the GenerateFuntion() to be able to use not volatile functions if verification is enabled
…RY and AGGREGATE_FUNCTION_ENTRY generation flow - if it's VOLATILE, we just should pick non-volatile function (but I didn't see there any volatile func for now; Also the GenerateOrderBy() for now uses GenerateStar(), but in the test output I still see not only the ORDER BY * (ex. BY COLUMN 🤔). What's more, with the seed = 1 and enable_verification=true it crashes during the Query 21 on the 13th attempt of setop->left = GenerateQueryNode();
fix indentation
… places generating like Order by something (*) and fix it

Add Generate Order by *, when verification is enabled. TODO: Find the places generating like Order by something (*) and fix it
…_enabled is false by default

when StatementGenerator object created from (*this), its verification_enabled is false by default

fix GenerateStarexpression to return only star expression, when verification_enabled=true

fix GenerateStarexpression to return only star expression, when verification_enabled=true

fix GenerateStarexpression to return only star expression, when verification_enabled=true

clean up

fix GenerateStarexpression to return only star expression, when verification_enabled=true
…ication_enabled=true

fix GenerateStarexpression to return only star expression, when verification_enabled=true

fix GenerateStarexpression to return only star expression, when verification_enabled=true

fix GenerateStarexpression to return only star expression, when verification_enabled=true
@hmeriann
Copy link
Contributor Author

@Tmonster could you please take a look at this PR?

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.

thanks! just a couple of comments

.gitmodules Show resolved Hide resolved
src/statement_generator.cpp Outdated Show resolved Hide resolved
@@ -532,6 +534,7 @@ unique_ptr<TableRef> StatementGenerator::GenerateSubqueryRef() {
unique_ptr<SelectStatement> subquery;
{
StatementGenerator child_generator(*this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can assign verification_enabled here in the constructor

Copy link
Contributor Author

@hmeriann hmeriann Jul 24, 2024

Choose a reason for hiding this comment

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

I created one more constructor to assign verification_enabled using it (and to avoid searching and adding the second parameter everywhere in the code).

src/statement_generator.cpp Show resolved Hide resolved
src/statement_generator.cpp Show resolved Hide resolved
src/statement_generator.cpp Outdated Show resolved Hide resolved
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.

Looks much better, just a few comments

src/statement_generator.cpp Show resolved Hide resolved
src/statement_generator.cpp Outdated Show resolved Hide resolved
@hmeriann hmeriann requested a review from Tmonster July 29, 2024 15:20
@Tmonster
Copy link
Collaborator

Tmonster commented Aug 7, 2024

Thanks!

@Tmonster Tmonster merged commit 9729cad into duckdb:main Aug 7, 2024
26 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