Skip to content

add NOT expression#600

Merged
sundy-li merged 15 commits into
databendlabs:masterfrom
jyizheng:support-not-expr
May 26, 2021
Merged

add NOT expression#600
sundy-li merged 15 commits into
databendlabs:masterfrom
jyizheng:support-not-expr

Conversation

@jyizheng

Copy link
Copy Markdown
Contributor

Summary

Support negation of boolean expression

Changelog

  • New Feature

Fixes #597

Test Plan

Unit Tests

@databend-bot databend-bot added the pr-feature this PR introduces a new feature to the codebase label May 23, 2021
@databend-bot

Copy link
Copy Markdown
Member

Thanks for the contribution!
I have applied any labels matching special text in your title and description.

Please review the labels and make any necessary changes.

@codecov-commenter

codecov-commenter commented May 23, 2021

Copy link
Copy Markdown

Codecov Report

Merging #600 (b432a3c) into master (c7e7f98) will increase coverage by 0%.
The diff coverage is 80%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #600   +/-   ##
======================================
  Coverage      80%     80%           
======================================
  Files         288     286    -2     
  Lines       13756   13729   -27     
======================================
+ Hits        11027   11052   +25     
+ Misses       2729    2677   -52     
Impacted Files Coverage Δ
...ery/src/datasources/system/functions_table_test.rs 92% <ø> (ø)
common/datavalues/src/data_array_logic.rs 62% <68%> (-5%) ⬇️
common/functions/src/logics/logic_test.rs 88% <83%> (-1%) ⬇️
...on/planners/src/plan_expression_action_function.rs 87% <83%> (-1%) ⬇️
common/datavalues/src/data_array_logic_test.rs 85% <87%> (-2%) ⬇️
common/datavalues/src/data_value_operator.rs 97% <100%> (+<1%) ⬆️
common/functions/src/logics/logic.rs 85% <100%> (+<1%) ⬆️
common/functions/src/logics/logic_not.rs 100% <100%> (ø)
common/planners/src/plan_expression_action_test.rs 95% <100%> (ø)
fusestore/store/src/meta_service/raftmeta.rs 65% <0%> (-1%) ⬇️
... and 11 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 c7e7f98...b432a3c. Read the comment docs.

@jyizheng
jyizheng marked this pull request as ready for review May 24, 2021 06:18
@jyizheng

Copy link
Copy Markdown
Contributor Author

@bohutang This PR is ready. Could you please review it?

@bohutang

Copy link
Copy Markdown
Member

Ok, ping @sundy-li for a review

@sundy-li

sundy-li commented May 24, 2021

Copy link
Copy Markdown
Member

Ok, Better after #550 , it will be finished in at most 2 days.

@sundy-li sundy-li self-assigned this May 24, 2021
@bohutang

Copy link
Copy Markdown
Member

Ok, Better after #550 , it will be finished in at most 2 days.

Ok, thanks @sundy-li
Cc @jyizheng

@jyizheng

Copy link
Copy Markdown
Contributor Author

@sundy-li Could you please review this PR?

@sundy-li

Copy link
Copy Markdown
Member

@sundy-li Could you please review this PR?

Ok, If your pr is not ready, you can create a draft PR. When it's ready, you can click ready for review button. Refer: https://github.blog/2019-02-14-introducing-draft-pull-requests/

@jyizheng

Copy link
Copy Markdown
Contributor Author

@sundy-li Could you please review this PR?

Ok, If your pr is not ready, you can create a draft PR. When it's ready, you can click ready for review button. Refer: https://github.blog/2019-02-14-introducing-draft-pull-requests/

Got it. It is ready now.

))
}
_ => Result::Err(ErrorCodes::BadDataValueType(format!(
"DataValue Error: Cannot do data_array Not, array:{:?}",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think DataColumnarValue::Constant case should be considered.

  1. Using val.to_array() , return the value as DataColumnarValue::Array
  2. Creating a array with just one element, and apply the compute::not, then return the value as DataColumnarValue::Constant

The second option can have better performance.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can have the stateless tests like select number from numbers(10) where not ( 5 )

@jyizheng jyizheng May 26, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestions. The function returns Result<DataArrayRef>, is it possible to return value as DataColumnarValue::Constant?

@sundy-li sundy-li May 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's ok as DataColumnarValue

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have changed the code. Could you review it again?

@sundy-li
sundy-li merged commit 1779661 into databendlabs:master May 26, 2021
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.

Support NOT expression

5 participants