Repository navigation
[GLUTEN-13416][FLINK] Support MD5 and SHA1 - #13420
zhang-jianq wants to merge 3 commits into
Conversation
49057af to
1ce3af8
Compare
7d4666f to
7d3d172
Compare
|
@KevinyhZou @lgbo-ustc @ggjh-159 Would you mind reviewing this PR? |
There was a problem hiding this comment.
Thanks for the minimal mapping and the end-to-end coverage.
- The repo's spotless check (bound to the build's validate phase) FAILED — HashFunctionsTest.java is missing the trailing newline at EOF. Please run
mvn spotless:apply(or add the final newline). - Please add the SQL details of the dual-run to the PR description — the queries in md5.sql/sha1.sql that were run against Flink 1.19.2.
2a2c0ca to
ad7c9e1
Compare
|
@ggjh-159 Thanks for the review. I have fixed both: |
95f411c to
ff527ea
Compare
ggjh-159
left a comment
There was a problem hiding this comment.
One comment on test coverage — details in the line comment on HashFunctionsTest.java.
| import java.util.List; | ||
|
|
||
| class HashFunctionsTest extends GlutenStreamingTestBase { | ||
|
|
There was a problem hiding this comment.
Please extend HashFunctionsTest with: empty-string and long-input rows for both MD5 and SHA1 (as in the dual-run set), a constant-input case (e.g. MD5('abc')), a nested-call case (e.g. MD5(UPPER(s))), and non-string input cases — MD5(CAST(n AS STRING)) over an int column plus the rejected bare MD5(n) asserting the validation error.
There was a problem hiding this comment.
Thanks for the review. Extended HashFunctionsTest to 8 symmetric cases (4 per function):
testMd5Basic/testSha1Basic— plain / multibyte / NULL plus a 32-char long input.testMd5Constant/testSha1Constant— literal inputs, including the empty string (MD5('')/SHA1('')). The empty string is covered via a literal because the values connector'sfrom_elementsdrops an empty-string row field, so an empty-string row can't be exercised through a table.testMd5Nested/testSha1Nested—MD5(LOWER(s))/SHA1(LOWER(s)). I usedLOWERrather thanUPPERbecauseUPPERis not registered in gluten yet.testMd5NonStringInput/testSha1NonStringInput—MD5(CAST(n AS STRING))/SHA1(CAST(n AS STRING))over an int column, plusassertThatThrownByon the bareMD5(n)/SHA1(n)asserting the FlinkValidationException.
|
@zhang-jianq please update velox4j version and Flink.md |
ff527ea to
d4dc72b
Compare
@ggjh-159 The velox4j-side PR (#68) is merged — |
b1a1f86 to
e6f1fa5
Compare
e6f1fa5 to
0a081d0
Compare
Signed-off-by: zhangjianqiu <1726110962@qq.com>
0a081d0 to
cd9f5a7
Compare
ggjh-159
left a comment
There was a problem hiding this comment.
Thanks for expanding the tests to the constant, nested, and non-string forms.
The PR description's "What changes" section still lists two changes that are no longer in the diff: "CI routing: .github/workflows/flink.yml clones the pinned velox4j branch" and "Docs: gluten-flink/docs/Flink.md build notes synced" — after the rebase onto main, the velox4j ref is managed by main's central gluten-flink/velox4j-ref.txt and the diff no longer touches either file. Please remove these two bullets.
[?] (no anchor) 一般——body——The PR description's "What changes" section still lists two changes that are no longer in the diff: "CI routing: .github/workflows/flink.yml clones the pinned velox4j branch" and "Docs: gluten-flink/docs/Flink.md build notes synced" — after the rebase onto main, the velox4j ref is managed by main's central gluten-flink/velox4j-ref.txt and the diff no longer touches either file. Please remove these two bullets.
@ggjh-159 Thanks for your review. It's fixed now. please review it again. |
What changes are proposed in this pull request?
Adds the planner-side wiring so Flink's
MD5(string)andSHA1(string)offload to the velox flinksqlmd5/sha1functions (bigo-sg/velox#87 adds the functions themselves):RexCallConverterFactory: map the MD5 and SHA1 RexCalls tomd5/sha1via the default name converter. Both take exactly one VARCHAR argument and return VARCHAR, so a plain name mapping suffices.HashFunctionsTestcovers end-to-end execution with 8 symmetric cases (4 per function) — basic / multibyte / NULL / long-input rows, literal inputs incl. the empty string, a nested call overLOWER, and non-string inputs (CAST(n AS STRING)plus the rejected bareMD5(n)/SHA1(n)). The MD5/SHA1 mappings are plain name mappings, so no separate converter unit test is added.How was this patch tested?
gluten-flink/ut:
HashFunctionsTest(8 cases — basic / multibyte / NULL / long-input, literal incl. empty string, nestedLOWER, and non-stringCAST/rejected bare int) passes.End-to-end dual-run on Flink 1.19.2 (native vs gluten-offloaded, identical VALUES inputs, changelog multiset exact comparison):
md5.sqlandsha1.sqlmatch native exactly (7 rows each).md5.sqldiffers only in projectingmd5(s)into amd5_scolumn; the query is:Related issue: #13416