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

datavalues2: Logic function #3998

Merged
merged 11 commits into from Jan 27, 2022

Conversation

Veeupup
Copy link
Collaborator

@Veeupup Veeupup commented Jan 26, 2022

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

datavalues2 logic function

  • and
  • or
  • xor
  • not

Changelog

  • New Feature

Related Issues

Fixes #3971

Test Plan

Unit Tests

Stateless Tests

@databend-bot
Copy link
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@databend-bot databend-bot added pr-feature this PR introduces a new feature to the codebase need-review labels Jan 26, 2022
@vercel
Copy link

vercel bot commented Jan 26, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/databend/databend/DNDooZXASSZShnyrSuWtMg9VxX1R
✅ Preview: Canceled

[Deployment for fdf91e2 canceled]

Signed-off-by: Veeupup <code@tanweime.com>
Signed-off-by: Veeupup <code@tanweime.com>
Signed-off-by: Veeupup <code@tanweime.com>
Signed-off-by: Veeupup <code@tanweime.com>
Signed-off-by: Veeupup <code@tanweime.com>
@Veeupup
Copy link
Collaborator Author

Veeupup commented Jan 26, 2022

Constant folding optimizer with And will produce unexcepted result, I comment it for now to make it work right.

In detail: null value will be optimized as false, so when select null and 1, it will return 0 rather than NULL, which is not excepted.
and we can remove unnecessary constant folding in another pr.

https://github.com/datafuselabs/databend/blob/12f5b58048/query/src/optimizers/optimizer_constant_folding.rs#L258-L265

@Veeupup Veeupup marked this pull request as ready for review January 26, 2022 15:22
};

for idx in 0..input_rows {
let (val, valid) = func(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can we make this func inline to have better performence? cc @sundy-li

Copy link
Member

Choose a reason for hiding this comment

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

Yes, better to test it vs before.

BTW, I do think func is dynamically dispatched without optimization.

I think it may be better to apply:

macro_rules! xxx;

match op {
LogicOperator::And => xxx!{},
...
}

Signed-off-by: Veeupup <code@tanweime.com>
Signed-off-by: Veeupup <code@tanweime.com>
@codecov-commenter
Copy link

Codecov Report

Merging #3998 (fdf91e2) into datavalues-dev (4187fd7) will increase coverage by 0%.
The diff coverage is 78%.

Impacted file tree graph

@@              Coverage Diff               @@
##           datavalues-dev   #3998   +/-   ##
==============================================
  Coverage              57%     57%           
==============================================
  Files                 812     813    +1     
  Lines               43937   44022   +85     
==============================================
+ Hits                25201   25255   +54     
- Misses              18736   18767   +31     
Impacted Files Coverage Δ
common/functions/src/scalars/function_factory.rs 94% <ø> (-1%) ⬇️
query/src/optimizers/optimizer_constant_folding.rs 79% <ø> (-7%) ⬇️
common/functions/src/scalars/logics/logic.rs 75% <75%> (+4%) ⬆️
common/functions/src/scalars/logics/and.rs 88% <85%> (ø)
common/functions/src/scalars/logics/or.rs 88% <85%> (ø)
common/functions/src/scalars/logics/not.rs 88% <88%> (ø)
common/functions/src/scalars/logics/xor.rs 88% <88%> (ø)
common/datavalues2/src/types/eq.rs 50% <100%> (ø)
common/functions/src/scalars/function2_factory.rs 88% <100%> (+<1%) ⬆️
common/datavalues/src/columns/logic.rs 0% <0%> (-86%) ⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edcba32...fdf91e2. Read the comment docs.

@databend-bot
Copy link
Member

CI Passed
Reviewers Approved
Let's Merge
Thank you for the PR @Veeupup

@databend-bot databend-bot merged commit 8a99e69 into datafuselabs:datavalues-dev Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants