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

add sign math function #19527

Merged
merged 5 commits into from
Jan 26, 2021
Merged

add sign math function #19527

merged 5 commits into from
Jan 26, 2021

Conversation

ucasfl
Copy link
Collaborator

@ucasfl ucasfl commented Jan 24, 2021

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Add sign math function.

@robot-clickhouse robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels Jan 24, 2021
@kitaisreal kitaisreal self-assigned this Jan 24, 2021
@kitaisreal
Copy link
Collaborator

@ucasfl could you please add documentation to this sign function ?

Copy link
Collaborator

@kitaisreal kitaisreal left a comment

Choose a reason for hiding this comment

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

Looks good to me.

ColumnWithTypeAndName right{zero_column, arguments[0].type, {}};

auto greater = FunctionFactory::instance().get("greater", context);
auto greater_compare = greater->build(ColumnsWithTypeAndName{left, right});
Copy link
Collaborator

@kitaisreal kitaisreal Jan 24, 2021

Choose a reason for hiding this comment

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

@ucasfl maybe it make sense to extract that ColumnsWithTypeAndName{left, right} in variable without creating it for each function execute in next lines ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@ucasfl
Copy link
Collaborator Author

ucasfl commented Jan 24, 2021

@ucasfl could you please add documentation to this sign function ?

Ok.

@kitaisreal
Copy link
Collaborator

kitaisreal commented Jan 25, 2021

@ucasfl after small discussion maybe it will be better to implement it using existing framework FunctionUnaryArithmetic?
Please check for example implementation of abs.cpp.
Also could you please add performance tests for this sign function ?

@ucasfl
Copy link
Collaborator Author

ucasfl commented Jan 26, 2021

@ucasfl after small discussion maybe it will be better to implement it using existing framework FunctionUnaryArithmetic?
Please check for example implementation of abs.cpp.
Also could you please add performance tests for this sign function ?

Ok, I have rewrite it with FunctionUnaryArithmetic.

@ucasfl
Copy link
Collaborator Author

ucasfl commented Jan 26, 2021

 01508_query_obfuscator:                                                 [ FAIL ] - result differs with reference:
2021-01-26 15:04:00 --- /ClickHouse/tests/queries/0_stateless/01508_query_obfuscator.reference	2021-01-26 14:36:59.361547309 +0300
2021-01-26 15:04:00 +++ /ClickHouse/tests/queries/0_stateless/01508_query_obfuscator.stdout	2021-01-26 15:04:00.586005177 +0300
2021-01-26 15:04:00 @@ -12,5 +12,5 @@
2021-01-26 15:04:00      AND MasonryID = 30750384
2021-01-26 15:04:00      AND intHash32(EyeballID) = 448362928 AND intHash64(EyeballID) = 12572659331310383983
2021-01-26 15:04:00      AND EarthquakeID IN (8195672321757027078, 7079643623150622129, 5057006826979676478, 7886875230160484653, 7494974311229040743)
2021-01-26 15:04:00 -    AND Photography = 1
2021-01-26 15:04:00 +    AND Sign = 1

I checked that obfuscator can't obfuscate reserve words like function name, so I made a little change on this test.

Copy link
Collaborator

@kitaisreal kitaisreal left a comment

Choose a reason for hiding this comment

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

@ucasfl looks good to me, lets wait for CI and merge.

@kitaisreal kitaisreal merged commit 81548e0 into ClickHouse:master Jan 26, 2021
@ucasfl ucasfl deleted the sign branch September 10, 2023 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature Pull request with new product feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants