[
https://issues.apache.org/jira/browse/FLINK-40265?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18100920#comment-18100920
]
Qiu Yanjun commented on FLINK-40265:
------------------------------------
Hi [~icshuo], I investigated this issue on the latest master (6ce85191d154) and
reproduced the representation-dependent ordering.
*Root cause*
{{BinaryStringData.compareTo}} has two comparison paths:
* When both instances retain {{javaObject}}, it calls {{String.compareTo}},
which compares UTF-16 code units.
* Otherwise it materializes the values and compares unsigned UTF-8 bytes.
For valid Unicode strings, these orders differ when an SMP code point is
compared with a BMP code point in U+E000-U+FFFF. For U+1F600 versus U+FF01, the
observed signs are:
{code}
UTF-8 bytes: f09f9880 versus efbc81
java/java = -1
binary/java = 1
java/binary = 1
binary/binary = 1
before serialization = -1
after serialization = 1
{code}
{{StringDataSerializer.serialize}} materializes UTF-8 bytes but leaves the
original Java object intact, while deserialization creates a binary-backed
value through {{StringData.fromBytes}}. Therefore a serializer round trip
changes which comparison path is selected. The original and deserialized values
compare equal to each other, but have opposite ordering relative to the same
third value, violating the {{Comparable}} consistency requirement.
*Proposed fix*
Use one canonical unsigned UTF-8 byte ordering for every backing
representation. The minimal fix is to remove the
{{javaObject.compareTo(other.javaObject)}} shortcut and always use the existing
materialized binary comparison. This matches the {{compareTo}} Javadoc and
{{SortUtil.putStringNormalizedKey}}, which already uses raw UTF-8 bytes. It
does not require a serialization-format or public-API change.
Because {{compareTo}} is on a sorting hot path, I would also evaluate the
Java-backed materialization cost. If preserving a no-allocation fast path is
necessary, it must implement ordering equivalent to encoded UTF-8, including
malformed-surrogate replacement behavior, rather than delegating to
{{String.compareTo}}.
*Test plan*
* Cover U+1F600 versus U+FF01 across Java/Java, Java/binary, binary/Java,
single-segment, and multi-segment representations.
* Assert that a {{StringDataSerializer}} round trip preserves the comparison
sign and the {{Comparable}} invariant.
* Assert that {{compareTo}} agrees with the string normalized-key order.
* Verify red/green behavior with {{BinaryStringDataTest}}.
One small correction to the reproduction snippet: it currently uses ASCII {{!}}
(U+0021). It should use {{\uFF01}} or the literal fullwidth character {{！}};
ASCII {{!}} does not expose the ordering difference.
Could you please assign FLINK-40265 to me? I would like to work on the fix
after confirming the approach. Thanks.
> BinaryStringData.compareTo uses inconsistent UTF-16 and UTF-8 ordering for
> SMP characters
> -----------------------------------------------------------------------------------------
>
> Key: FLINK-40265
> URL: https://issues.apache.org/jira/browse/FLINK-40265
> Project: Flink
> Issue Type: Bug
> Components: Table SQL / Runtime
> Reporter: Shuo Cheng
> Priority: Major
>
> {{BinaryStringData.compareTo}} uses two different comparison paths:
> * If both values have a non-null {{javaObject}}, it delegates to
> {{String.compareTo}}, which compares UTF-16 code units.
> * Otherwise, it materializes both values and compares their UTF-8 bytes as
> unsigned bytes.
> These orders are not equivalent for comparisons between an SMP character and
> BMP characters in the range U+E000-U+FFFF. As a result, the comparison result
> depends on whether a {{BinaryStringData}} is Java-backed or binary-backed and
> can change after a serializer round trip.
> For example, compare GRINNING FACE (😀, U+1F600) with FULLWIDTH EXCLAMATION
> MARK (！, U+FF01):
> * UTF-16: D83D DE00 < FF01, so 😀 < ！
> * UTF-8: F0 9F 98 80 > EF BC 81, so 😀 > ！
> h3. Reproduce
> {code:java}
> @Test
> void compareToIsConsistentAfterSerialization() throws IOException {
> BinaryStringData grinningFace = BinaryStringData.fromString("😀");
> BinaryStringData fullWidthExclamationMark =
> BinaryStringData.fromString("!");
> DataOutputSerializer output = new DataOutputSerializer(16);
> StringDataSerializer.INSTANCE.serialize(grinningFace, output);
> BinaryStringData deserializedGrinningFace =
> (BinaryStringData)
> StringDataSerializer.INSTANCE.deserialize(
> new
> DataInputDeserializer(output.getCopyOfBuffer()));
> assertThat(deserializedGrinningFace).isEqualTo(grinningFace);
>
> assertThat(deserializedGrinningFace.compareTo(fullWidthExclamationMark)).isPositive();
> assertThat(grinningFace.compareTo(fullWidthExclamationMark))
>
> .isEqualTo(deserializedGrinningFace.compareTo(fullWidthExclamationMark));
> }
> {code}
> h3. Actual behavior
> After serialization:
> * {{grinningFace.compareTo(fullWidthExclamationMark)}} is negative because
> both values retain Java objects and use {{String.compareTo}}.
> * {{deserializedGrinningFace.compareTo(fullWidthExclamationMark)}} is
> positive because the deserialized value is binary-backed and comparison uses
> UTF-8 bytes.
> The final assertion fails.
> h3. Expected behavior
> {{BinaryStringData.compareTo}} should produce the same ordering regardless of
> the backing representation and before/after serialization. The class Javadoc
> and {{SortUtil.putStringNormalizedKey}} already describe/use UTF-8 byte
> ordering, so comparison should follow one canonical ordering.
> h3. Impact
> This violates the {{Comparable}} contract. The original and deserialized
> values compare equal to each other, but have opposite ordering relative to
> the same third value. It can cause representation-dependent results in
> generated string comparisons, sorting, MIN/MAX, or other operations using
> {{compareTo}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
