Skip to content

[nereids](function)fix encryption function translation bug#15038

Merged
morrySnow merged 2 commits intoapache:masterfrom
starocean999:master_bug_1031
Dec 20, 2022
Merged

[nereids](function)fix encryption function translation bug#15038
morrySnow merged 2 commits intoapache:masterfrom
starocean999:master_bug_1031

Conversation

@starocean999
Copy link
Copy Markdown
Contributor

@starocean999 starocean999 commented Dec 13, 2022

Proposed changes

Issue Number: close #xxx

Problem summary

Describe your changes.

Checklist(Required)

  1. Does it affect the original behavior:
    • Yes
    • No
    • I don't know
  2. Has unit tests been added:
    • Yes
    • No
    • No Need
  3. Has document been added or modified:
    • Yes
    • No
    • No Need
  4. Does it need to update dependencies:
    • Yes
    • No
  5. Are there any changes that cannot be rolled back:
    • Yes (If Yes, please explain WHY)
    • No

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

@hello-stephen
Copy link
Copy Markdown
Contributor

hello-stephen commented Dec 13, 2022

TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 36.23 seconds
load time: 675 seconds
storage size: 17123719449 Bytes
https://doris-community-test-1308700295.cos.ap-hongkong.myqcloud.com/tmp/20221219045000_clickbench_pr_64889.html

@starocean999 starocean999 force-pushed the master_bug_1031 branch 2 times, most recently from a700263 to 9d70dce Compare December 15, 2022 01:13
Comment on lines +420 to +421
String blockEncryptionMode = context.getRuntimeTranslator().get().getRuntimeFilterContext()
.getSessionVariable().getBlockEncryptionMode();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if u want session variables, u has two way to do that:

  1. ConnectContext.get(). getSessionVariable()
  2. put cascadesContext into PlanTranslatorContext

.getSessionVariable().getBlockEncryptionMode();
if (function.getName().equalsIgnoreCase("aes_decrypt")
|| function.getName().equalsIgnoreCase("aes_encrypt")) {
HashSet<String> aesModes = new HashSet<>(Arrays.asList(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should put this set to an util class and make it static

Comment on lines +456 to +457
} else if (function.getName().equalsIgnoreCase("sm4_decrypt")
|| function.getName().equalsIgnoreCase("sm4_encrypt")) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe we need a new interface for all en/decrypt functions, the interface has two use:

  1. we can use xxx instanceOf xxx, instead of use function name to judge function type
  2. afford some helper function, such as
    a. isAes()
    b. isSm4()

@924060929
Copy link
Copy Markdown
Contributor

I think we should create new function like checkArguments in the new ScalarFunction, and do the check in the analysis stage

@github-actions
Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actions github-actions bot added approved Indicates a PR has been approved by one committer. reviewed labels Dec 19, 2022
@github-actions
Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@morrySnow morrySnow merged commit 6712f1f into apache:master Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. area/nereids kind/test reviewed

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants