Re: [PR] HIVE-29514: Optimize UDF Unhex and improve its test coverage [hive]
soumyakanti3578 merged PR #6471: URL: https://github.com/apache/hive/pull/6471 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HIVE-29514: Optimize UDF Unhex and improve its test coverage [hive]
soumyakanti3578 commented on code in PR #6471:
URL: https://github.com/apache/hive/pull/6471#discussion_r3314954239
##
ql/src/test/org/apache/hadoop/hive/ql/udf/TestUDFUnhex.java:
##
@@ -40,8 +42,41 @@ public void testUnhexConversion(){
UDFUnhex udf = new UDFUnhex();
byte[] output = udf.evaluate(hex);
assertEquals(expected.length,output.length);
-for (int i = 0; i < expected.length; i++){
- assertEquals(expected[i], output[i]);
-}
+assertArrayEquals(expected, output);
+ }
+
+ @Test
+ public void testUnhexOddLength() {
+UDFUnhex udf = new UDFUnhex();
+
+Text hex1 = new Text("A");
+byte[] expected1 = new byte[] {(byte) 0x0A};
+assertArrayEquals(expected1, udf.evaluate(hex1));
+
+Text hex2 = new Text("123");
+byte[] expected2 = new byte[] {(byte) 0x01, (byte) 0x23};
+assertArrayEquals(expected2, udf.evaluate(hex2));
+ }
+
+ @Test
+ public void testUnhexInvalidCharacters() {
+UDFUnhex udf = new UDFUnhex();
+
+Text hex = new Text("7374G9");
+assertNull("Should return null for invalid hex characters",
udf.evaluate(hex));
+
+Text hexOddInvalid = new Text("12G");
+assertNull("Should return null for invalid hex characters in odd length
string", udf.evaluate(hexOddInvalid));
+ }
+
+ @Test
+ public void testUnhexNullEmptyCases() {
+UDFUnhex udf = new UDFUnhex();
+
+assertNull(udf.evaluate(null));
+
+Text hexEmpty = new Text("");
+byte[] expectedEmpty = new byte[0];
+assertArrayEquals(expectedEmpty, udf.evaluate(hexEmpty));
}
Review Comment:
Yeah I don't think `HEX(UNHEX(...))` is really valid.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HIVE-29514: Optimize UDF Unhex and improve its test coverage [hive]
sonarqubecloud[bot] commented on PR #6471: URL: https://github.com/apache/hive/pull/6471#issuecomment-4522101166 ## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6471) **Quality Gate passed** Issues  [4 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6471&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6471&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6471&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6471&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6471&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6471) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HIVE-29514: Optimize UDF Unhex and improve its test coverage [hive]
tanishq-chugh commented on code in PR #6471:
URL: https://github.com/apache/hive/pull/6471#discussion_r3290564817
##
ql/src/test/org/apache/hadoop/hive/ql/udf/TestUDFUnhex.java:
##
@@ -40,8 +42,41 @@ public void testUnhexConversion(){
UDFUnhex udf = new UDFUnhex();
byte[] output = udf.evaluate(hex);
assertEquals(expected.length,output.length);
-for (int i = 0; i < expected.length; i++){
- assertEquals(expected[i], output[i]);
-}
+assertArrayEquals(expected, output);
+ }
+
+ @Test
+ public void testUnhexOddLength() {
+UDFUnhex udf = new UDFUnhex();
+
+Text hex1 = new Text("A");
+byte[] expected1 = new byte[] {(byte) 0x0A};
+assertArrayEquals(expected1, udf.evaluate(hex1));
+
+Text hex2 = new Text("123");
+byte[] expected2 = new byte[] {(byte) 0x01, (byte) 0x23};
+assertArrayEquals(expected2, udf.evaluate(hex2));
+ }
+
+ @Test
+ public void testUnhexInvalidCharacters() {
+UDFUnhex udf = new UDFUnhex();
+
+Text hex = new Text("7374G9");
+assertNull("Should return null for invalid hex characters",
udf.evaluate(hex));
+
+Text hexOddInvalid = new Text("12G");
+assertNull("Should return null for invalid hex characters in odd length
string", udf.evaluate(hexOddInvalid));
+ }
+
+ @Test
+ public void testUnhexNullEmptyCases() {
+UDFUnhex udf = new UDFUnhex();
+
+assertNull(udf.evaluate(null));
+
+Text hexEmpty = new Text("");
+byte[] expectedEmpty = new byte[0];
+assertArrayEquals(expectedEmpty, udf.evaluate(hexEmpty));
}
Review Comment:
Have added these test cases in commit:
[254027f](https://github.com/apache/hive/pull/6471/commits/254027f42517e7e1fda76d355c1ef31aade3b4ae)
>Also, maybe not in this file, but are there any tests for hex(unhex(...))
and unhex(hex(..))?
I see the `UNHEX(HEX(...))` case being used in `ba_table_udfs.q` QTest, but
i don't think `HEX(UNHEX(...))` is a valid scenario, what do you think? please
feel free to correct me if i am wrong here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HIVE-29514: Optimize UDF Unhex and improve its test coverage [hive]
tanishq-chugh commented on code in PR #6471:
URL: https://github.com/apache/hive/pull/6471#discussion_r3290541170
##
ql/src/java/org/apache/hadoop/hive/ql/udf/UDFUnhex.java:
##
@@ -42,32 +42,59 @@
public class UDFUnhex extends UDF {
/**
- * Convert every two hex digits in s into.
- *
+ * Convert every two hex digits in s into a byte.
*/
public byte[] evaluate(Text s) {
if (s == null) {
return null;
}
-// append a leading 0 if needed
-String str;
-if (s.getLength() % 2 == 1) {
- str = "0" + s.toString();
-} else {
- str = s.toString();
+int len = s.getLength();
+if (len == 0) {
+ return new byte[0];
}
-byte[] result = new byte[str.length() / 2];
-for (int i = 0; i < str.length(); i += 2) {
- try {
-result[i / 2] = ((byte) Integer.parseInt(str.substring(i, i + 2), 16));
- } catch (NumberFormatException e) {
-// invalid character present, return null
+byte[] textBytes = s.getBytes();
+
+// (len + 1) / 2 ensures right size for odd lengths
+byte[] result = new byte[(len + 1) / 2];
+
+int i = 0;
+int resIdx = 0;
+
+// If length is odd, the first character acts as the first byte avoiding
adding "0" prefix
+if (len % 2 != 0) {
+ int val = decodeHexChar(textBytes[i++]);
+ if (val == -1) {
+return null;
+ }
+ result[resIdx++] = (byte) val;
+}
+
+while (i < len) {
+ int high = decodeHexChar(textBytes[i++]);
+ int low = decodeHexChar(textBytes[i++]);
+
+ if (high == -1 || low == -1) {
return null;
}
+
+ result[resIdx++] = (byte) ((high << 4) | low);
}
Review Comment:
that would be better for optimisation, have updated the loop in commit:
[318630a](https://github.com/apache/hive/pull/6471/commits/318630a163306c926b9cd6eb4dd5a43c8d6c08fb)
Have added the test cases for high/low in commit:
[d93c476](https://github.com/apache/hive/pull/6471/commits/d93c476db0d893fd620b9d96e5e0076002df1f57)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HIVE-29514: Optimize UDF Unhex and improve its test coverage [hive]
tanishq-chugh commented on code in PR #6471:
URL: https://github.com/apache/hive/pull/6471#discussion_r3290536941
##
ql/src/java/org/apache/hadoop/hive/ql/udf/UDFUnhex.java:
##
@@ -42,32 +42,59 @@
public class UDFUnhex extends UDF {
/**
- * Convert every two hex digits in s into.
- *
+ * Convert every two hex digits in s into a byte.
*/
public byte[] evaluate(Text s) {
if (s == null) {
return null;
}
-// append a leading 0 if needed
-String str;
-if (s.getLength() % 2 == 1) {
- str = "0" + s.toString();
-} else {
- str = s.toString();
+int len = s.getLength();
+if (len == 0) {
+ return new byte[0];
}
-byte[] result = new byte[str.length() / 2];
-for (int i = 0; i < str.length(); i += 2) {
- try {
-result[i / 2] = ((byte) Integer.parseInt(str.substring(i, i + 2), 16));
- } catch (NumberFormatException e) {
-// invalid character present, return null
+byte[] textBytes = s.getBytes();
+
+// (len + 1) / 2 ensures right size for odd lengths
+byte[] result = new byte[(len + 1) / 2];
+
+int i = 0;
+int resIdx = 0;
+
+// If length is odd, the first character acts as the first byte avoiding
adding "0" prefix
+if (len % 2 != 0) {
+ int val = decodeHexChar(textBytes[i++]);
+ if (val == -1) {
+return null;
+ }
Review Comment:
Hi @soumyakanti3578 , Thanks for checking this!
Added the case in commit:
[a530e08](https://github.com/apache/hive/pull/6471/commits/a530e083b55cf8ee5df198698d28a41b709e9329)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HIVE-29514: Optimize UDF Unhex and improve its test coverage [hive]
soumyakanti3578 commented on code in PR #6471:
URL: https://github.com/apache/hive/pull/6471#discussion_r3270517152
##
ql/src/test/org/apache/hadoop/hive/ql/udf/TestUDFUnhex.java:
##
@@ -40,8 +42,41 @@ public void testUnhexConversion(){
UDFUnhex udf = new UDFUnhex();
byte[] output = udf.evaluate(hex);
assertEquals(expected.length,output.length);
-for (int i = 0; i < expected.length; i++){
- assertEquals(expected[i], output[i]);
-}
+assertArrayEquals(expected, output);
+ }
+
+ @Test
+ public void testUnhexOddLength() {
+UDFUnhex udf = new UDFUnhex();
+
+Text hex1 = new Text("A");
+byte[] expected1 = new byte[] {(byte) 0x0A};
+assertArrayEquals(expected1, udf.evaluate(hex1));
+
+Text hex2 = new Text("123");
+byte[] expected2 = new byte[] {(byte) 0x01, (byte) 0x23};
+assertArrayEquals(expected2, udf.evaluate(hex2));
+ }
+
+ @Test
+ public void testUnhexInvalidCharacters() {
+UDFUnhex udf = new UDFUnhex();
+
+Text hex = new Text("7374G9");
+assertNull("Should return null for invalid hex characters",
udf.evaluate(hex));
+
+Text hexOddInvalid = new Text("12G");
+assertNull("Should return null for invalid hex characters in odd length
string", udf.evaluate(hexOddInvalid));
+ }
+
+ @Test
+ public void testUnhexNullEmptyCases() {
+UDFUnhex udf = new UDFUnhex();
+
+assertNull(udf.evaluate(null));
+
+Text hexEmpty = new Text("");
+byte[] expectedEmpty = new byte[0];
+assertArrayEquals(expectedEmpty, udf.evaluate(hexEmpty));
}
Review Comment:
It would be nice to have tests for
- mixed case: `aABb9`
- boundary values
- lower case, as all tests are for upper case right now
Also, maybe not in this file, but are there any tests for `hex(unhex(...))`
and `unhex(hex(..))`?
##
ql/src/java/org/apache/hadoop/hive/ql/udf/UDFUnhex.java:
##
@@ -42,32 +42,59 @@
public class UDFUnhex extends UDF {
/**
- * Convert every two hex digits in s into.
- *
+ * Convert every two hex digits in s into a byte.
*/
public byte[] evaluate(Text s) {
if (s == null) {
return null;
}
-// append a leading 0 if needed
-String str;
-if (s.getLength() % 2 == 1) {
- str = "0" + s.toString();
-} else {
- str = s.toString();
+int len = s.getLength();
+if (len == 0) {
+ return new byte[0];
}
-byte[] result = new byte[str.length() / 2];
-for (int i = 0; i < str.length(); i += 2) {
- try {
-result[i / 2] = ((byte) Integer.parseInt(str.substring(i, i + 2), 16));
- } catch (NumberFormatException e) {
-// invalid character present, return null
+byte[] textBytes = s.getBytes();
+
+// (len + 1) / 2 ensures right size for odd lengths
+byte[] result = new byte[(len + 1) / 2];
+
+int i = 0;
+int resIdx = 0;
+
+// If length is odd, the first character acts as the first byte avoiding
adding "0" prefix
+if (len % 2 != 0) {
+ int val = decodeHexChar(textBytes[i++]);
+ if (val == -1) {
+return null;
+ }
Review Comment:
Please add a test for invalid character in an odd-length input.
Example: `G`.
##
ql/src/java/org/apache/hadoop/hive/ql/udf/UDFUnhex.java:
##
@@ -42,32 +42,59 @@
public class UDFUnhex extends UDF {
/**
- * Convert every two hex digits in s into.
- *
+ * Convert every two hex digits in s into a byte.
*/
public byte[] evaluate(Text s) {
if (s == null) {
return null;
}
-// append a leading 0 if needed
-String str;
-if (s.getLength() % 2 == 1) {
- str = "0" + s.toString();
-} else {
- str = s.toString();
+int len = s.getLength();
+if (len == 0) {
+ return new byte[0];
}
-byte[] result = new byte[str.length() / 2];
-for (int i = 0; i < str.length(); i += 2) {
- try {
-result[i / 2] = ((byte) Integer.parseInt(str.substring(i, i + 2), 16));
- } catch (NumberFormatException e) {
-// invalid character present, return null
+byte[] textBytes = s.getBytes();
+
+// (len + 1) / 2 ensures right size for odd lengths
+byte[] result = new byte[(len + 1) / 2];
+
+int i = 0;
+int resIdx = 0;
+
+// If length is odd, the first character acts as the first byte avoiding
adding "0" prefix
+if (len % 2 != 0) {
+ int val = decodeHexChar(textBytes[i++]);
+ if (val == -1) {
+return null;
+ }
+ result[resIdx++] = (byte) val;
+}
+
+while (i < len) {
+ int high = decodeHexChar(textBytes[i++]);
+ int low = decodeHexChar(textBytes[i++]);
+
+ if (high == -1 || low == -1) {
return null;
}
+
+ result[resIdx++] = (byte) ((high << 4) | low);
}
Review Comment:
How about:
```
while (i < len) {
int high, low;
if ((high = decodeHexChar(textBytes[i++])) == -1 ||
(low = decodeHexChar(te
Re: [PR] HIVE-29514: Optimize UDF Unhex and improve its test coverage [hive]
sonarqubecloud[bot] commented on PR #6471: URL: https://github.com/apache/hive/pull/6471#issuecomment-4418096917 ## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6471) **Quality Gate passed** Issues  [1 New issue](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6471&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6471&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6471&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6471&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6471&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6471) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HIVE-29514: Optimize UDF Unhex and improve its test coverage [hive]
sonarqubecloud[bot] commented on PR #6471: URL: https://github.com/apache/hive/pull/6471#issuecomment-4416310276 ## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6471) **Quality Gate passed** Issues  [1 New issue](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6471&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6471&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6471&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6471&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6471&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6471) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HIVE-29514: Optimize UDF Unhex and improve its test coverage [hive]
sonarqubecloud[bot] commented on PR #6471: URL: https://github.com/apache/hive/pull/6471#issuecomment-4415730285 ## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6471) **Quality Gate passed** Issues  [3 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6471&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6471&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6471&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6471&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6471&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6471) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
[PR] HIVE-29514: Optimize UDF Unhex and improve its test coverage [hive]
tanishq-chugh opened a new pull request, #6471: URL: https://github.com/apache/hive/pull/6471 ### What changes were proposed in this pull request? Improve the UDF Unhex and its test coverage ### Why are the changes needed? Better performance ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Manual testing + java test -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
