You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "zhongyujiang (via GitHub)" <gi...@apache.org> on 2024/09/18 14:32:42 UTC
[PR] Core: Fix UnicodeUtil#truncateStringMax returns malformed string. [iceberg]
zhongyujiang opened a new pull request, #11161:
URL: https://github.com/apache/iceberg/pull/11161
We encountered an exception while writing data, and the stack trace is as follows. It occurred during the collection of Parquet column metrics:
#### Exception stack:
```
Suppressed: org.apache.iceberg.exceptions.RuntimeIOException: Failed to encode value as UTF-8: Ҋ�Qڞ<֔~�MECڮV?
at org.apache.iceberg.types.Conversions.toByteBuffer(Conversions.java:110)
at org.apache.iceberg.types.Conversions.toByteBuffer(Conversions.java:83)
at org.apache.iceberg.parquet.ParquetUtil.toBufferMap(ParquetUtil.java:343)
at org.apache.iceberg.parquet.ParquetUtil.footerMetrics(ParquetUtil.java:174)
at org.apache.iceberg.parquet.ParquetUtil.footerMetrics(ParquetUtil.java:86)
at org.apache.iceberg.parquet.ParquetWriter.metrics(ParquetWriter.java:166)
at org.apache.iceberg.io.DataWriter.close(DataWriter.java:100)
at org.apache.iceberg.io.RollingFileWriter.closeCurrentWriter(RollingFileWriter.java:122)
at org.apache.iceberg.io.RollingFileWriter.close(RollingFileWriter.java:147)
at org.apache.iceberg.io.RollingDataWriter.close(RollingDataWriter.java:32)
at org.apache.iceberg.io.FanoutWriter.closeWriters(FanoutWriter.java:82)
at org.apache.iceberg.io.FanoutWriter.close(FanoutWriter.java:74)
at org.apache.iceberg.io.FanoutDataWriter.close(FanoutDataWriter.java:31)
at org.apache.iceberg.spark.source.SparkWrite$PartitionedDataWriter.close(SparkWrite.java:1162)
at org.apache.spark.sql.execution.datasources.v2.DataWritingSparkTask$.$anonfun$run$9(WriteToDataSourceV2Exec.scala:423)
at org.apache.spark.util.Utils$.tryWithSafeFinallyAndFailureCallbacks(Utils.scala:1496)
... 10 more
Caused by: java.nio.charset.MalformedInputException: Input length = 1
at java.nio.charset.CoderResult.throwException(CoderResult.java:281)
at java.nio.charset.CharsetEncoder.encode(CharsetEncoder.java:816)
at org.apache.iceberg.types.Conversions.toByteBuffer(Conversions.java:108)
... 25 more
Caused by: java.nio.charset.MalformedInputException: Input length = 1
at java.nio.charset.CoderResult.throwException(CoderResult.java:281)
at java.nio.charset.CharsetEncoder.encode(CharsetEncoder.java:816)
at org.apache.iceberg.types.Conversions.toByteBuffer(Conversions.java:108)
```
#### Investigation
After some investigation, I found that when collecting Parquet column metrics, string metrics are truncated by default to a length of 16 characters. When truncating the max metric, if the truncated length is less than the original max value, the last character is incremented by 1 to ensure that the truncated value is greater than the max value. However, this increment operation did not consider skipping illegal UTF-8 Unicode code points, which led to the following exception.
In the scenario where we encountered this issue, there is a Parquet file with a column's max metric length exceeding 16, and the code point of its 16th character is '\uD7FF', which is `Character.MIN_SURROGATE - 1`. Adding 1 to this resulted in `Character.MIN_SURROGATE`, which is not a [valid Unicode scalar value](https://www.unicode.org/versions/Unicode16.0.0/core-spec/chapter-3/#G27288). Therefore, when `Conversions.toByteBuffer` attempted to encode it in UTF-8 format, a `MalformedInputException` was thrown.
This fix specifically skips illegal code points when incrementing the last character to avoid this issue.
#### To reproduce
```sql
CREATE TABLE my_table (data string) using iceberg;
INSERT INTO my_table VALUES('abcdefghigklmno\uD7FFp');
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix UnicodeUtil#truncateStringMax returns malformed string. [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11161:
URL: https://github.com/apache/iceberg/pull/11161#discussion_r1767604741
##########
api/src/main/java/org/apache/iceberg/util/UnicodeUtil.java:
##########
@@ -93,4 +93,24 @@ public static Literal<CharSequence> truncateStringMax(Literal<CharSequence> inpu
}
return null; // Cannot find a valid upper bound
}
+
+ private static int incrementCodePoint(int codePoint) {
+ // surrogate code points are not Unicode scalar values,
+ // any UTF-8 byte sequence that would otherwise map to code points U+D800..U+DFFF is ill-formed.
+ // see https://www.unicode.org/versions/Unicode16.0.0/core-spec/chapter-3/#G27288
+ Preconditions.checkArgument(
Review Comment:
Just a minor point here, but shouldn't this only be relevant if we somehow get non-unicode binary in a unicode string? Shouldn't be possible in a Java string right?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix UnicodeUtil#truncateStringMax returns malformed string. [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11161:
URL: https://github.com/apache/iceberg/pull/11161#discussion_r1767608468
##########
core/src/test/java/org/apache/iceberg/TestMetricsTruncation.java:
##########
@@ -274,4 +274,17 @@ public void testTruncateStringMax() {
"Test input with multiple 4 byte UTF-8 character where the first unicode character should be incremented")
.isEqualTo(0);
}
+
+ @Test
+ public void testTruncateStringMaxUpperBound() {
Review Comment:
Could we add these to the test above? I'm also fine if there is a specific reason to have them somewhere else but it seems like these would fit into the test above as just other examples. The test cases for +1 and MAX_CODE_POINT are there already right?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix UnicodeUtil#truncateStringMax returns malformed string. [iceberg]
Posted by "zhongyujiang (via GitHub)" <gi...@apache.org>.
zhongyujiang commented on PR #11161:
URL: https://github.com/apache/iceberg/pull/11161#issuecomment-2359897318
@amogh-jahagirdar @nastra can you please help review this? thanks.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix UnicodeUtil#truncateStringMax returns malformed string. [iceberg]
Posted by "zhongyujiang (via GitHub)" <gi...@apache.org>.
zhongyujiang commented on code in PR #11161:
URL: https://github.com/apache/iceberg/pull/11161#discussion_r1770763130
##########
core/src/test/java/org/apache/iceberg/TestMetricsTruncation.java:
##########
@@ -274,4 +274,17 @@ public void testTruncateStringMax() {
"Test input with multiple 4 byte UTF-8 character where the first unicode character should be incremented")
.isEqualTo(0);
}
+
+ @Test
+ public void testTruncateStringMaxUpperBound() {
Review Comment:
Moved this to `testTruncateStringMax`
> The test cases for +1 and MAX_CODE_POINT are there already right?
Yes, there is already one test case for MAX_CODE_POINT
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix UnicodeUtil#truncateStringMax returns malformed string. [iceberg]
Posted by "zhongyujiang (via GitHub)" <gi...@apache.org>.
zhongyujiang commented on code in PR #11161:
URL: https://github.com/apache/iceberg/pull/11161#discussion_r1770762980
##########
api/src/main/java/org/apache/iceberg/util/UnicodeUtil.java:
##########
@@ -93,4 +93,24 @@ public static Literal<CharSequence> truncateStringMax(Literal<CharSequence> inpu
}
return null; // Cannot find a valid upper bound
}
+
+ private static int incrementCodePoint(int codePoint) {
+ // surrogate code points are not Unicode scalar values,
+ // any UTF-8 byte sequence that would otherwise map to code points U+D800..U+DFFF is ill-formed.
+ // see https://www.unicode.org/versions/Unicode16.0.0/core-spec/chapter-3/#G27288
+ Preconditions.checkArgument(
Review Comment:
It is possible for Java strings to contain only one unpaired surrogate character(non-unicode character), though encoding them using UTF-8 or UTF-16 will result in `MalformedInputException`. This is also the case in this issue, where the truncation method returns a string ending with an unpaired high-surrogate character, but fails when encoding it to UTF-8.
For a valid UTF-8 string, it will not contain unpaired surrogates. However, the `codePointAt` [method](https://docs.oracle.com/javase/8/docs/api/java/lang/StringBuilder.html#codePointAt-int-) may return a unpaired surrogate code point if an incorrect index is passed.
> /**
* Returns the character (Unicode code point) at the specified
* index. The index refers to {@code char} values
* (Unicode code units) and ranges from {@code 0} to
* {@link #length()}{@code - 1}.
*
* <p> If the {@code char} value specified at the given index
* is in the high-surrogate range, the following index is less
* than the length of this sequence, and the
* {@code char} value at the following index is in the
* low-surrogate range, then the supplementary code point
* corresponding to this surrogate pair is returned. Otherwise,
* the {@code char} value at the given index is returned.
*
* @param index the index to the {@code char} values
* @return the code point value of the character at the
* {@code index}
* @throws IndexOutOfBoundsException if the {@code index}
* argument is negative or not less than the length of this
* sequence.
*/
public int codePointAt(int index) {
Currently, all methods in the `UnicodeUtil` class that use `codePointAt` are correct and will not result in an unpaired surrogate code point. I added it to strengthen the validation.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix UnicodeUtil#truncateStringMax returns malformed string. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11161:
URL: https://github.com/apache/iceberg/pull/11161#discussion_r1770808088
##########
core/src/test/java/org/apache/iceberg/TestMetricsTruncation.java:
##########
@@ -273,5 +282,23 @@ public void testTruncateStringMax() {
.as(
"Test input with multiple 4 byte UTF-8 character where the first unicode character should be incremented")
.isEqualTo(0);
+
+ assertThat(cmp.compare(truncateStringMax(Literal.of(test8), 2).value(), test8))
+ .as("Truncated upper bound should be greater than or equal to the actual upper bound")
+ .isGreaterThanOrEqualTo(0);
+ assertThat(cmp.compare(truncateStringMax(Literal.of(test8), 2).value(), test8_2_expected))
+ .as(
+ "Test the last character is the 4-byte max UTF-8 character after truncated where the second-to-last "
+ + "character should be incremented")
+ .isEqualTo(0);
+
+ assertThat(cmp.compare(truncateStringMax(Literal.of(test9), 2).value(), test9))
+ .as("Truncated upper bound should be greater than or equal to the actual upper bound")
+ .isGreaterThanOrEqualTo(0);
+
+ assertThat(cmp.compare(truncateStringMax(Literal.of(test9), 2).value(), test9_2_expected))
Review Comment:
this is missing an actuall assertion check
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix UnicodeUtil#truncateStringMax returns malformed string. [iceberg]
Posted by "zhongyujiang (via GitHub)" <gi...@apache.org>.
zhongyujiang commented on code in PR #11161:
URL: https://github.com/apache/iceberg/pull/11161#discussion_r1770764300
##########
core/src/test/java/org/apache/iceberg/TestMetricsTruncation.java:
##########
@@ -202,11 +202,20 @@ public void testTruncateStringMax() {
String test5 = "\uDBFF\uDFFF\uDBFF\uDFFF";
String test6 = "\uD800\uDFFF\uD800\uDFFF";
// Increment the previous character
- String test6_2_expected = "\uD801\uDC00";
+ String test6_1_expected = "\uD801\uDC00";
Review Comment:
I think this should be a typo, "\uD800\uDFFF" is a Unicode surrogate pair, it's length is 1.
##########
core/src/test/java/org/apache/iceberg/TestMetricsTruncation.java:
##########
@@ -202,11 +202,20 @@ public void testTruncateStringMax() {
String test5 = "\uDBFF\uDFFF\uDBFF\uDFFF";
String test6 = "\uD800\uDFFF\uD800\uDFFF";
// Increment the previous character
- String test6_2_expected = "\uD801\uDC00";
+ String test6_1_expected = "\uD801\uDC00";
String test7 = "\uD83D\uDE02\uD83D\uDE02\uD83D\uDE02";
String test7_2_expected = "\uD83D\uDE02\uD83D\uDE03";
String test7_1_expected = "\uD83D\uDE03";
+ // Increment the max UTF-8 character will overflow
+ String test8 = "a\uDBFF\uDFFFc";
+ String test8_2_expected = "b";
Review Comment:
The characters in `test5` are all `MAX_CODE_POINT`, so the upper bound does not exist.
`test8` adds a case where an overflow occurs due to `MAX_CODE_POINT`, but it is possible to increment the previous character to get an upper bound.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix UnicodeUtil#truncateStringMax returns malformed string. [iceberg]
Posted by "zhongyujiang (via GitHub)" <gi...@apache.org>.
zhongyujiang commented on code in PR #11161:
URL: https://github.com/apache/iceberg/pull/11161#discussion_r1770813666
##########
core/src/test/java/org/apache/iceberg/TestMetricsTruncation.java:
##########
@@ -273,5 +282,23 @@ public void testTruncateStringMax() {
.as(
"Test input with multiple 4 byte UTF-8 character where the first unicode character should be incremented")
.isEqualTo(0);
+
+ assertThat(cmp.compare(truncateStringMax(Literal.of(test8), 2).value(), test8))
+ .as("Truncated upper bound should be greater than or equal to the actual upper bound")
+ .isGreaterThanOrEqualTo(0);
+ assertThat(cmp.compare(truncateStringMax(Literal.of(test8), 2).value(), test8_2_expected))
+ .as(
+ "Test the last character is the 4-byte max UTF-8 character after truncated where the second-to-last "
+ + "character should be incremented")
+ .isEqualTo(0);
+
+ assertThat(cmp.compare(truncateStringMax(Literal.of(test9), 2).value(), test9))
+ .as("Truncated upper bound should be greater than or equal to the actual upper bound")
+ .isGreaterThanOrEqualTo(0);
+
+ assertThat(cmp.compare(truncateStringMax(Literal.of(test9), 2).value(), test9_2_expected))
Review Comment:
Fixed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org