Skip to content

API: Add tests for UnicodeUtil - #17528

Open
uros-b wants to merge 2 commits into
apache:mainfrom
uros-b:test-add-unicodeutil-coverage
Open

API: Add tests for UnicodeUtil#17528
uros-b wants to merge 2 commits into
apache:mainfrom
uros-b:test-add-unicodeutil-coverage

Conversation

@uros-b

@uros-b uros-b commented Aug 5, 2026

Copy link
Copy Markdown
Member

UnicodeUtil had no dedicated test class. This adds tests covering its public surface: isCharHighSurrogate at both ends of the high-surrogate range (0xD800-0xDBFF), plus a low surrogate and an ASCII char; truncateString when no truncation is needed, when truncating by code points, and that a surrogate pair (emoji) is not split; the IllegalArgumentException guard for a non-positive truncate length; truncateStringMin / truncateStringMax, including the overflow case where every retained code point is the maximum and no greater bound exists (returns null). Test-only changes.

@github-actions github-actions Bot added the API label Aug 5, 2026
@uros-b
uros-b force-pushed the test-add-unicodeutil-coverage branch from a8d5383 to e591870 Compare August 5, 2026 10:08
Comment thread api/src/test/java/org/apache/iceberg/util/TestUnicodeUtil.java Outdated
Cover isCharHighSurrogate, truncateString (including surrogate pairs and the non-positive length guard), and truncateStringMin/Max, including the overflow case where no greater bound exists.
@uros-b
uros-b force-pushed the test-add-unicodeutil-coverage branch from e591870 to 84b4161 Compare August 5, 2026 16:15

@szehon-ho szehon-ho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good — test-only coverage for UnicodeUtil in the owning module, assertions match the implementation, and CI is green. Approving.

Optional follow-ups (not blocking): the String overload of truncateStringMax could also cover two paths that are already exercised for the Literal overload in TestMetricsTruncation:

  1. No truncation needed (early return when the input already fits):
assertThat(UnicodeUtil.truncateStringMax("abc", 5)).isEqualTo("abc");
  1. Carry when the last retained code point overflows:
String input =
    new StringBuilder()
        .append('a')
        .appendCodePoint(Character.MAX_CODE_POINT)
        .append('c')
        .toString();
assertThat(UnicodeUtil.truncateStringMax(input, 2)).isEqualTo("b");

@ebyhr ebyhr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think there is one more uncovered path in UnicodeUtil.incrementCodePoint L127. When the last retained code point is \uD7FF (one below MIN_SURROGATE), the method skips over the surrogate block and returns \uE000.

  @Test
  void truncateStringMaxSkipsSurrogateRange() {
    // \uD7FF + 1 must skip the surrogate block (U+D800..U+DFFF) and return U+E000
    String input = "\uD7FF" + "extra";
    assertThat(UnicodeUtil.truncateStringMax(input, 1)).isEqualTo("\uE000");
  }

Comment thread api/src/test/java/org/apache/iceberg/util/TestUnicodeUtil.java
Comment thread api/src/test/java/org/apache/iceberg/util/TestUnicodeUtil.java
@uros-b

uros-b commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Thank you folks @szehon-ho @ebyhr @nastra!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants