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

feat: Table evolution fuzzer #11872

Closed
wants to merge 1 commit into from
Closed

feat: Table evolution fuzzer #11872

wants to merge 1 commit into from

Conversation

Yuhta
Copy link
Contributor

@Yuhta Yuhta commented Dec 16, 2024

Summary: Basic version of table evolution fuzzer. Full design of the fuzzer can be found at https://docs.google.com/document/d/18jjNRknSxI99mgdL7eDMzkq65i-mi47Ukymi3JkXzUA/edit

Differential Revision: D67283632

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 16, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67283632

@netlify Netlify
Copy link

netlify bot commented Dec 16, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 92f241f
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6762eeb4a520540008f1639c

Yuhta added a commit to Yuhta/velox that referenced this pull request Dec 16, 2024
@Yuhta @facebook-github-bot
feat: Table evolution fuzzer (facebookincubator#11872)
6154a11 
Summary:

Basic version of table evolution fuzzer.  Full design of the fuzzer can be found at https://docs.google.com/document/d/18jjNRknSxI99mgdL7eDMzkq65i-mi47Ukymi3JkXzUA/edit

Differential Revision: D67283632
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67283632

Yuhta added a commit to Yuhta/velox that referenced this pull request Dec 16, 2024
@Yuhta @facebook-github-bot
feat: Table evolution fuzzer (facebookincubator#11872)
506aa7d 
Summary:

Basic version of table evolution fuzzer.  Full design of the fuzzer can be found at https://docs.google.com/document/d/18jjNRknSxI99mgdL7eDMzkq65i-mi47Ukymi3JkXzUA/edit

Differential Revision: D67283632
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67283632

Yuhta added a commit to Yuhta/velox that referenced this pull request Dec 17, 2024
@Yuhta @facebook-github-bot
feat: Table evolution fuzzer (facebookincubator#11872)
623c129 
Summary:

Basic version of table evolution fuzzer.  Full design of the fuzzer can be found at https://docs.google.com/document/d/18jjNRknSxI99mgdL7eDMzkq65i-mi47Ukymi3JkXzUA/edit

Differential Revision: D67283632
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67283632

Yuhta added a commit to Yuhta/velox that referenced this pull request Dec 17, 2024
@Yuhta @facebook-github-bot
feat: Table evolution fuzzer (facebookincubator#11872)
ff7504d 
Summary:

Basic version of table evolution fuzzer.  Full design of the fuzzer can be found at https://docs.google.com/document/d/18jjNRknSxI99mgdL7eDMzkq65i-mi47Ukymi3JkXzUA/edit

Differential Revision: D67283632
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67283632

Yuhta added a commit to Yuhta/nimble that referenced this pull request Dec 17, 2024
@Yuhta @facebook-github-bot
feat: Table evolution fuzzer
0738a51 
Summary:
X-link: facebookincubator/velox#11872

Basic version of table evolution fuzzer.  Full design of the fuzzer can be found at https://docs.google.com/document/d/18jjNRknSxI99mgdL7eDMzkq65i-mi47Ukymi3JkXzUA/edit

Differential Revision: D67283632
@Yuhta Yuhta mentioned this pull request Dec 17, 2024
Closed
xiaoxmeng
xiaoxmeng reviewed Dec 18, 2024
View reviewed changes
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@Yuhta LGTM % minors. Thanks!

velox/exec/Task.cpp Show resolved Hide resolved
velox/exec/tests/TableEvolutionFuzzerTest.cpp Show resolved Hide resolved
velox/exec/tests/TableEvolutionFuzzerTest.cpp Outdated Show resolved Hide resolved
velox/exec/tests/TableEvolutionFuzzerTest.cpp Show resolved Hide resolved
velox/exec/tests/TableEvolutionFuzzerTest.cpp Show resolved Hide resolved
velox/exec/tests/TableEvolutionFuzzerTest.cpp Outdated Show resolved Hide resolved
velox/exec/tests/TableEvolutionFuzzerTest.cpp
for (int i = 0; i < config_.evolutionCount; ++i) {
auto data = vectorFuzzer_.fuzzRow(setups[i].schema, kVectorSize, false);
auto actualDir = fmt::format("{}/actual_{}", tableDir->getPath(), i);
VELOX_CHECK(std::filesystem::create_directory(actualDir));
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use velox fs to create?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just one fewer dependency since testing that component is not in the scope of this fuzzer

velox/exec/tests/TableEvolutionFuzzerTest.cpp Show resolved Hide resolved
velox/exec/tests/TableEvolutionFuzzerTest.cpp Show resolved Hide resolved
velox/exec/tests/TableEvolutionFuzzerTest.cpp Show resolved Hide resolved
Yuhta added a commit to Yuhta/nimble that referenced this pull request Dec 18, 2024
@Yuhta @facebook-github-bot
feat: Table evolution fuzzer (facebookincubator#116)
b82def5 
Summary:

X-link: facebookincubator/velox#11872

Basic version of table evolution fuzzer.  Full design of the fuzzer can be found at https://docs.google.com/document/d/18jjNRknSxI99mgdL7eDMzkq65i-mi47Ukymi3JkXzUA/edit

Differential Revision: D67283632
Yuhta added a commit to Yuhta/velox that referenced this pull request Dec 18, 2024
@Yuhta @facebook-github-bot
feat: Table evolution fuzzer (facebookincubator#11872)
0e18012 
Summary:
X-link: facebookincubator/nimble#116


Basic version of table evolution fuzzer.  Full design of the fuzzer can be found at https://docs.google.com/document/d/18jjNRknSxI99mgdL7eDMzkq65i-mi47Ukymi3JkXzUA/edit

Differential Revision: D67283632
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67283632

@Yuhta @facebook-github-bot
feat: Table evolution fuzzer (facebookincubator#11872)
92f241f 
Summary:
X-link: facebookincubator/nimble#116


Basic version of table evolution fuzzer.  Full design of the fuzzer can be found at https://docs.google.com/document/d/18jjNRknSxI99mgdL7eDMzkq65i-mi47Ukymi3JkXzUA/edit

Differential Revision: D67283632
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67283632

xiaoxmeng
xiaoxmeng approved these changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@Yuhta thanks for the update!

facebook-github-bot pushed a commit to facebookincubator/nimble that referenced this pull request Dec 18, 2024
@Yuhta @facebook-github-bot
feat: Table evolution fuzzer (#116)
8ddc37c 
Summary:
Pull Request resolved: #116

X-link: facebookincubator/velox#11872

Basic version of table evolution fuzzer.  Full design of the fuzzer can be found at https://docs.google.com/document/d/18jjNRknSxI99mgdL7eDMzkq65i-mi47Ukymi3JkXzUA/edit

Reviewed By: xiaoxmeng

Differential Revision: D67283632

fbshipit-source-id: c41ac3cfde75eec68ec86a5fa43936cd2bcd608b
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 2f81755.

kagamiori
kagamiori reviewed Dec 24, 2024
View reviewed changes
velox/vector/fuzzer/tests/VectorFuzzerTest.cpp
Comment on lines +502 to +503
opts.allowDictionaryVector = false;
opts.allowConstantVector = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why we set these options to false here? They were implicitly true before this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is not well written and they assume the top level vector is not encoded in constant or dictionary. I changed the random number generator to a faster implementation and these assumption no longer holds. We can fix the test to decode instead of use as<> cast.

velox/exec/tests/TableEvolutionFuzzerTest.cpp
auto names = old.names();
auto types = old.children();
for (int i = 0, j = 0; i < old.size(); ++i) {
// Skip evolving bucket column.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it not possible of bucket column being evolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No the Hive partition function gives different hashes for same value in REAL vs in DOUBLE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kagamiori kagamiori kagamiori left review comments

@xiaoxmeng xiaoxmeng xiaoxmeng approved these changes

@assignUser assignUser Awaiting requested review from assignUser assignUser is a code owner

@majetideepak majetideepak Awaiting requested review from majetideepak majetideepak is a code owner

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Yuhta @facebook-github-bot @kagamiori @xiaoxmeng