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

refactor(fuzzer): add supporting map functions #11536

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor(fuzzer): add supporting map functions #11536

wants to merge 1 commit into from
+51 −13

Conversation

wuray22
Copy link

@wuray22 wuray22 commented Nov 14, 2024 •
edited
Loading

Summary: This diff adds some supporting functions for my diff D65926722 for new benchmark tests for metalake's map_concat. In my benchmark tests, I want to be able to control the density of map columns. And generate the number of columns given a range, using the same seed.

Differential Revision: D65755011

@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 Nov 14, 2024
@netlify Netlify
Copy link

netlify bot commented Nov 14, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 9350ee2
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67436ad88217980008b78e57

@facebook-github-bot
Copy link
Contributor

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

@wuray22 wuray22 changed the title Velox fuzzer - add supporting map functions test(fuzzer): add supporting map functions Nov 14, 2024
wuray22 added a commit to wuray22/velox that referenced this pull request Nov 20, 2024
@wuray22 @facebook-github-bot
test(fuzzer): add supporting map functions (facebookincubator#11536)
50d6c09 
Summary:

This diff adds some supporting functions for my diff for new benchmark tests for map_concat.

Reviewed By: darrenfu

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

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

wuray22 added a commit to wuray22/velox that referenced this pull request Nov 20, 2024
@wuray22 @facebook-github-bot
test(fuzzer): add supporting map functions (facebookincubator#11536)
a331929 
Summary:

This diff adds some supporting functions for my diff for new benchmark tests for map_concat.

Reviewed By: darrenfu

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

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

wuray22 added a commit to wuray22/velox that referenced this pull request Nov 20, 2024
@wuray22 @facebook-github-bot
test(fuzzer): add supporting map functions (facebookincubator#11536)
e42a835 
Summary:

This diff adds some supporting functions for my diff for new benchmark tests for map_concat.

Reviewed By: darrenfu

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

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

wuray22 added a commit to wuray22/velox that referenced this pull request Nov 20, 2024
@wuray22 @facebook-github-bot
test(fuzzer): add supporting map functions (facebookincubator#11536)
0015d7f 
Summary:

This diff adds some supporting functions for my diff for new benchmark tests for map_concat.

Reviewed By: darrenfu

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

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

wuray22 added a commit to wuray22/velox that referenced this pull request Nov 20, 2024
@wuray22 @facebook-github-bot
test(fuzzer): add supporting map functions (facebookincubator#11536)
4cb733c 
Summary:

This diff adds some supporting functions for my diff for new benchmark tests for map_concat.

Reviewed By: darrenfu

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

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

@Yuhta Yuhta requested a review from kagamiori November 22, 2024 22:32
kagamiori
kagamiori reviewed Nov 23, 2024
View reviewed changes
Copy link
Contributor

@kagamiori kagamiori left a comment

Choose a reason for hiding this comment

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

Hi @wuray22, thank you for making the contribution! I think the type of this PR is more of a refactoring, so the title can be "refactor(fuzzer): ...". Also, could you add more explanation about why it's beneficial to make this change? E.g., it expose xxx API, makes xxx easier to use, etc.

velox/vector/fuzzer/VectorFuzzer.cpp Outdated Show resolved Hide resolved
velox/vector/fuzzer/VectorFuzzer.cpp Outdated Show resolved Hide resolved
velox/vector/fuzzer/VectorFuzzer.cpp Outdated Show resolved Hide resolved
velox/vector/fuzzer/VectorFuzzer.cpp Outdated
Comment on lines 942 to 944
RowTypePtr VectorFuzzer::randRowType(size_t numFields, int maxDepth) {
return velox::randRowType(rng_, numFields, maxDepth);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this method used?

velox/vector/fuzzer/VectorFuzzer.cpp
@@ -912,6 +925,10 @@ TypePtr VectorFuzzer::randType(
return velox::randType(rng_, scalarTypes, maxDepth);
}

TypePtr VectorFuzzer::randMapType(int maxDepth) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this method used?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. This function is used in my follow up diff D65926722 for controlling the number of maps in my row vector

wuray22 added a commit to wuray22/velox that referenced this pull request Nov 24, 2024
@wuray22 @facebook-github-bot
test(fuzzer): add supporting map functions (facebookincubator#11536)
1d8aafd 
Summary:

This diff adds some supporting functions for my diff for new benchmark tests for map_concat.

Reviewed By: darrenfu

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

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

wuray22 added a commit to wuray22/velox that referenced this pull request Nov 24, 2024
@wuray22 @facebook-github-bot
test(fuzzer): add supporting map functions (facebookincubator#11536)
5e20018 
Summary:

This diff adds some supporting functions for my diff for new benchmark tests for map_concat.

Reviewed By: darrenfu

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

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

@wuray22 @facebook-github-bot
test(fuzzer): add supporting map functions (facebookincubator#11536)
9350ee2 
Summary:

This diff adds some supporting functions for my diff for new benchmark tests for map_concat.

Reviewed By: darrenfu

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

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

@wuray22 wuray22 changed the title test(fuzzer): add supporting map functions refactor(fuzzer): add supporting map functions Nov 24, 2024
wuray22
wuray22 commented Nov 24, 2024
View reviewed changes
Copy link
Author

@wuray22 wuray22 left a comment

Choose a reason for hiding this comment

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

Thanks for the review @kagamiori . I addressed your comments and updated the description.

velox/vector/fuzzer/VectorFuzzer.cpp
@@ -912,6 +925,10 @@ TypePtr VectorFuzzer::randType(
return velox::randType(rng_, scalarTypes, maxDepth);
}

TypePtr VectorFuzzer::randMapType(int maxDepth) {
Copy link
Author

Choose a reason for hiding this comment

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

Yes. This function is used in my follow up diff D65926722 for controlling the number of maps in my row vector

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

@kagamiori kagamiori Awaiting requested review from kagamiori

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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wuray22 @facebook-github-bot @kagamiori