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

Adds checks for verifying internal state of a vector #5885

Conversation

bikramSingh91
Copy link
Contributor

@bikramSingh91 bikramSingh91 commented Jul 27, 2023

This change adds thorough checks for each vector type, including its
children vectors. These checks verify the validity of buffer sizes
and their values (if applicable), such as nulls, values, and indices,
offsets, etc. Currently, its only used in debug builds on the result
of expressions and the objective is to catch these issues especially
in the expression fuzzer. These checks help clarify expected states
and enforce them, improving overall vector reliability.

This check also found a bug in Combinations(Array<>) udf which is
fixed as a part of this change.

Test Plan:
Ran fuzzer for 1 hour

@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 Jul 27, 2023
@netlify
Copy link

netlify bot commented Jul 27, 2023

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit ae47c87
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/64d145e13170ab00085ff65a

@bikramSingh91 bikramSingh91 force-pushed the addVectorIntegrityChecks branch 2 times, most recently from b60f3b1 to 8f5a7f6 Compare July 27, 2023 22:04
@facebook-github-bot
Copy link
Contributor

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@bikramSingh91 bikramSingh91 force-pushed the addVectorIntegrityChecks branch 2 times, most recently from fa651ed to cea7956 Compare July 31, 2023 23:35
@facebook-github-bot
Copy link
Contributor

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

velox/vector/BaseVector.h Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

velox/vector/BaseVector.h Outdated Show resolved Hide resolved
velox/vector/ComplexVector.cpp Outdated Show resolved Hide resolved
velox/vector/ComplexVector.h Outdated Show resolved Hide resolved
velox/vector/ConstantVector.h Show resolved Hide resolved
velox/vector/FlatVector.cpp Outdated Show resolved Hide resolved
velox/vector/FlatVector.h Outdated Show resolved Hide resolved
velox/vector/DictionaryVector-inl.h Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

velox/vector/BaseVector.h Outdated Show resolved Hide resolved
This change adds thorough checks for each vector type, including its
children vectors. These checks verify the validity of buffer sizes
and their values (if applicable), such as nulls, values, and indices,
offsets, etc. Currently, its only used in debug builds on the result
of expressions and the objective is to catch these issues especially
in the expression fuzzer. These checks help clarify expected states
and enforce them, improving overall vector reliability.

This check also found a bug in Combinations(Array<>) udf which is
fixed as a part of this change.

Test Plan:
Ran fuzzer for 1 hour
@facebook-github-bot
Copy link
Contributor

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@bikramSingh91 merged this pull request in 8ce030f.

@conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit 8ce030f0.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

unigof pushed a commit to unigof/velox that referenced this pull request Aug 18, 2023
…or#5885)

Summary:
This change adds thorough checks for each vector type, including its
children vectors. These checks verify the validity of buffer sizes
and their values (if applicable), such as nulls, values, and indices,
offsets, etc. Currently, its only used in debug builds on the result
of expressions and the objective is to catch these issues especially
in the expression fuzzer. These checks help clarify expected states
and enforce them, improving overall vector reliability.

This check also found a bug in Combinations(Array<>) udf which is
fixed as a part of this change.

Pull Request resolved: facebookincubator#5885

Test Plan: Ran fuzzer for 1 hour

Reviewed By: Yuhta

Differential Revision: D47856464

Pulled By: bikramSingh91

fbshipit-source-id: 03dc82137eb8c2691561af041053bb8cb6e4bc99
bikramSingh91 pushed a commit to bikramSingh91/velox that referenced this pull request Aug 29, 2023
Summary:
This fixes a bug where the string buffers were not acquired from the
input to the UDF. Caught by facebookincubator#5885

Differential Revision: D48800126

fbshipit-source-id: a6138be8393985c4da3324a50a57c880484b4180
bikramSingh91 pushed a commit to bikramSingh91/velox that referenced this pull request Aug 30, 2023
Summary:
Pull Request resolved: facebookincubator#6336

This fixes a bug where the string buffers were not acquired from the
input to the UDF. Caught by facebookincubator#5885

Differential Revision: D48800126

fbshipit-source-id: 325b0c78b18843ac0a15688e68a2011080f42805
bikramSingh91 pushed a commit to bikramSingh91/velox that referenced this pull request Aug 30, 2023
Summary:
Pull Request resolved: facebookincubator#6336

This fixes a bug where the string buffers were not acquired from the
input to the UDF. Caught by facebookincubator#5885

Differential Revision: D48800126

fbshipit-source-id: 1a57dabcb72120ba99d2cded702075e9afbbf1d1
bikramSingh91 pushed a commit to bikramSingh91/velox that referenced this pull request Aug 30, 2023
Summary:
Pull Request resolved: facebookincubator#6336

This fixes a bug where the string buffers were not acquired from the
input to the UDF. Caught by facebookincubator#5885

Differential Revision: D48800126

fbshipit-source-id: a0d631f68bb388c82cc8a3bc5928394ca3614789
facebook-github-bot pushed a commit that referenced this pull request Sep 1, 2023
Summary:
Pull Request resolved: #6336

This fixes a bug where the string buffers were not acquired from the
input to the UDF. Caught by #5885

Reviewed By: kgpai

Differential Revision: D48800126

fbshipit-source-id: 4f070cabfa9c2c40c5b8ca0b0444afebb7922d7e
codyschierbeck pushed a commit to codyschierbeck/velox that referenced this pull request Sep 27, 2023
Summary:
Pull Request resolved: facebookincubator#6336

This fixes a bug where the string buffers were not acquired from the
input to the UDF. Caught by facebookincubator#5885

Reviewed By: kgpai

Differential Revision: D48800126

fbshipit-source-id: 4f070cabfa9c2c40c5b8ca0b0444afebb7922d7e
codyschierbeck pushed a commit to codyschierbeck/velox that referenced this pull request Sep 27, 2023
Summary:
Pull Request resolved: facebookincubator#6336

This fixes a bug where the string buffers were not acquired from the
input to the UDF. Caught by facebookincubator#5885

Reviewed By: kgpai

Differential Revision: D48800126

fbshipit-source-id: 4f070cabfa9c2c40c5b8ca0b0444afebb7922d7e
codyschierbeck pushed a commit to codyschierbeck/velox that referenced this pull request Sep 27, 2023
Summary:
Pull Request resolved: facebookincubator#6336

This fixes a bug where the string buffers were not acquired from the
input to the UDF. Caught by facebookincubator#5885

Reviewed By: kgpai

Differential Revision: D48800126

fbshipit-source-id: 4f070cabfa9c2c40c5b8ca0b0444afebb7922d7e
ericyuliu pushed a commit to ericyuliu/velox that referenced this pull request Oct 12, 2023
…or#5885)

Summary:
This change adds thorough checks for each vector type, including its
children vectors. These checks verify the validity of buffer sizes
and their values (if applicable), such as nulls, values, and indices,
offsets, etc. Currently, its only used in debug builds on the result
of expressions and the objective is to catch these issues especially
in the expression fuzzer. These checks help clarify expected states
and enforce them, improving overall vector reliability.

This check also found a bug in Combinations(Array<>) udf which is
fixed as a part of this change.

Pull Request resolved: facebookincubator#5885

Test Plan: Ran fuzzer for 1 hour

Reviewed By: Yuhta

Differential Revision: D47856464

Pulled By: bikramSingh91

fbshipit-source-id: 03dc82137eb8c2691561af041053bb8cb6e4bc99
ericyuliu pushed a commit to ericyuliu/velox that referenced this pull request Oct 12, 2023
Summary:
Pull Request resolved: facebookincubator#6336

This fixes a bug where the string buffers were not acquired from the
input to the UDF. Caught by facebookincubator#5885

Reviewed By: kgpai

Differential Revision: D48800126

fbshipit-source-id: 4f070cabfa9c2c40c5b8ca0b0444afebb7922d7e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants