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

FIX: Refactor computeMetaData and separate propagateNull() from computePropagateNulls() API. #5287

Closed

Conversation

laithsakka
Copy link
Contributor

Summary:
This fix a set of expression fuzzer failures such as #5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:

  • The re-compute that happens over and over during query compilations.
  • Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: D46457021

@netlify
Copy link

netlify bot commented Jun 15, 2023

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 5ba1868
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/64952d24dd25870008194478

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported labels Jun 15, 2023
@facebook-github-bot
Copy link
Contributor

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

1 similar comment
@facebook-github-bot
Copy link
Contributor

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

laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 21, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: D46457021

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

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

laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 21, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: 393b3a34a4f3e32c176354f49d6498cbcfe60baa
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 21, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: ca89f41cc8e3b68017a53beb966d5f4524b928e1
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 21, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Reviewed By: kagamiori

Differential Revision: D46457021

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

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

laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 21, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: 3a05abefb8f9fce1b58897085d7f8611e56bedcd
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 21, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: d43f3931d2a4d4b3c2b9faa0766be68d0a08141f
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 21, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Reviewed By: kagamiori

Differential Revision: D46457021

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

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

laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 21, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: 1764f2f7a3d89e4f67b573b09f95e4c33565f29b
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 21, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: f18b11cc44c3c1fc754856c99a5556db752b5cee
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 21, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: e1190599d10e5ba60262c735e50d6c145bd904bd
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 22, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Reviewed By: kagamiori

Differential Revision: D46457021

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

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

laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 22, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: 9000dbc5e256a000bcec5ef8cd016b8c96db830c
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 22, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Reviewed By: kagamiori

Differential Revision: D46457021

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

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

laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 22, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: d1b38e078747155057ac58cf64a0d28b081dc9dd
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 22, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: fe0cf1e2a9786a6b0551b90436f115177a3be454
kagamiori pushed a commit to kagamiori/velox that referenced this pull request Jun 22, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: 4cb7532a1113eb652f90f17c563090a24780ef7b
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 22, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Reviewed By: kagamiori

Differential Revision: D46457021

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

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

laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 22, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: 100dc594ec64ca728d1d025918ba8dca362b6bd1
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 22, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: b694458e8eeedf7960307284b0297869b69c86fa
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 22, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Reviewed By: kagamiori

Differential Revision: D46457021

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

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

laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 22, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: 32a4b05f855f01d3f12ac98e2a200e94eaf148f7
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 22, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Reviewed By: kagamiori

Differential Revision: D46457021

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

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

laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 22, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: 970ec75cf67fbd088b01f656125618e4fa9a25f0
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 23, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Reviewed By: kagamiori

Differential Revision: D46457021

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

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

laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 23, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: 76acf23ee2ff17a1f4d579a8ece08e7e14e0925d
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 23, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

fbshipit-source-id: fac2da18ffb263cc7b838c8d724fca6b840cb05f
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Reviewed By: kagamiori

Differential Revision: D46457021

fbshipit-source-id: 9303e67b909632dad48523597c80b7d36ddac402
laithsakka added a commit to laithsakka/velox that referenced this pull request Jun 23, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

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

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

kagamiori pushed a commit to kagamiori/velox that referenced this pull request Jun 23, 2023
…tePropagateNulls() API. (facebookincubator#5287)

Summary:
Pull Request resolved: facebookincubator#5287

This fix a set of expression fuzzer failures such as facebookincubator#5059.

The diff split propagateNull() and computePropagateNulls() API. previously propagateNull()
used to retrieve the propagateNull property for some expressions and re-compute it for others.

with this change propagateNull() only accesses the pre-computed property. And
computePropagateNulls() is called from computeMetaData to compute the property
for some expressions.

The diff also refactor computeMetaData for better readability, it used to interleave computations
of different properties and combine things in a hard to track manner.

Following diffs will address two remaining issues:
- The re-compute that happens over and over during query compilations.
- Avoid emptying distinct fields if shared with parents and simplify the API.

Differential Revision: https://www.internalfb.com/diff/D46457021?entry_point=27

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

This pull request has been merged in dc1aa9a.

@conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit dc1aa9ab.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

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. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants