Skip to content
This repository has been archived by the owner on Jun 7, 2021. It is now read-only.

[TRAFODION-2229] add Hashing functions to Trafodion #813

Merged
merged 4 commits into from
Nov 8, 2016

Conversation

moscowgentalman
Copy link
Contributor

Compatibility enhancement
add new functions for MySQL:
CRC32() Compute a cyclic redundancy check value
MD5() Calculate MD5 checksum
SHA1(), SHA() Calculate an SHA-1 160-bit checksum
SHA2() Calculate an SHA-2 checksum

Unit test is to test several input value, and invoke same query in MySQL, and compare the result. Pass the test since results are same.

@Traf-Jenkins
Copy link

Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1318/

@Traf-Jenkins
Copy link

@zellerh
Copy link
Contributor

zellerh commented Nov 3, 2016

+1
Looks good to me. Did you follow the guide that Dave Birdsall put together a while ago for adding new functions?

Copy link
Contributor

@DaveBirdsall DaveBirdsall left a comment

Choose a reason for hiding this comment

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

Nice set of changes. There are a few issues that should be fixed, and some other suggestions that might improve the code.

}


| TOK_SHA1 '(' value_expression ')'
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this production is in there twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks Dave for catching this, this is a bug, will fix.

1, $3);
}

| TOK_SHA2 '(' value_expression ')'
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this production is here twice?

ITM_MD5 = 2635,
ITM_SHA1 = 2636,
ITM_SHA2 = 2637,
ITM_SHA = 2638,
Copy link
Contributor

Choose a reason for hiding this comment

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

ITM_SHA is never used. Looking at the parser changes, it appears that SHA is simply a syntactic sugar meaning the same as SHA1, so the difference between SHA and SHA1 disappears after parsing. So ITM_SHA is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can refactor the code to remove ITM_SHA

FUNC_MD5_ID = 123,
FUNC_SHA1_ID = 124,
FUNC_SHA2_ID = 125,
FUNC_SHA_ID = 126
Copy link
Contributor

Choose a reason for hiding this comment

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

FUNC_SHA_ID is never used and should be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix

@@ -40,6 +40,10 @@


#include <math.h>
#include <zlib.h>
#include "openssl/md5.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering why this isn't <openssl/md5.h>. Seems like one would have to copy the md5.h file into the source tree somewhere for this to compile cleanly. Maybe you meant to use angle brackets instead of quotes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix

SHA256_Init(&sha_ctx);
SHA256_Update(&sha_ctx, op_data[1], slen);
SHA256_Final((unsigned char*) sha,&sha_ctx);
char tmp[2];
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks too short. The sprintf will write three bytes (two digits + a null terminator), so it will overrun this buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, will fix

SHA1_Init(&sha_ctx);
SHA1_Update(&sha_ctx, op_data[1], slen);
SHA1_Final((unsigned char*) sha,&sha_ctx);
char tmp[2];
Copy link
Contributor

Choose a reason for hiding this comment

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

This buffer is too short.

MD5_Init(&md5_ctx);
MD5_Update(&md5_ctx, op_data[1], slen);
MD5_Final((unsigned char*) md5,&md5_ctx);
char tmp[2];
Copy link
Contributor

Choose a reason for hiding this comment

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

This buffer is too short.

@Traf-Jenkins
Copy link

New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1324/

@Traf-Jenkins
Copy link

@moscowgentalman
Copy link
Contributor Author

@DaveBirdsall please help to review again, I update according to your comments. And add a few regression tests.

@selvaganesang
Copy link
Contributor

+1 Nice change

@moscowgentalman
Copy link
Contributor Author

Thanks all for the review, I will merge this if no other comments. I fixed all previous comments in last commit.

@DaveBirdsall
Copy link
Contributor

I will take a quick look now...

Copy link
Contributor

@DaveBirdsall DaveBirdsall left a comment

Choose a reason for hiding this comment

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

+1 Looks great!

@asfgit asfgit merged commit 63e5338 into apache:master Nov 8, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants