[SPARK-43117][CONNECT] Make ProtoUtils.abbreviate support repeated fields

[zhengruifeng]

What changes were proposed in this pull request?

Make ProtoUtils.abbreviate support repeated fields

Why are the changes needed?

existing implementation does not work for repeated fields (strings/messages)

we don't have repeated bytes in Spark Connect for now, so let it alone

Does this PR introduce any user-facing change?

no

How was this patch tested?

added UTs

Was this patch authored or co-authored using generative AI tooling?

no

[LuciferYang]

nit: Do we need to consider the scenario where strings.iterator().hasNext is false?

[LuciferYang]

Can string really be null?

[zhengruifeng]

I guess so, but not very sure.
So I added string != null to protect from NPE

[LuciferYang]

In fact, for repeated fields, null checks exist whether adding a single element or adding a collection of elements, for example:

    public Builder addColumnNames(
        java.lang.String value) {
      if (value == null) { throw new NullPointerException(); }
      ensureColumnNamesIsMutable();
      columnNames_.add(value);
      bitField0_ |= 0x00000004;
      onChanged();
      return this;
    }
// com.google.protobuf.AbstractMessageLite.Builder#addAll(java.lang.Iterable<T>, java.util.List<? super T>)
protected static <T> void addAll(final Iterable<T> values, final List<? super T> list) {
      checkNotNull(values);
      if (values instanceof LazyStringList) {
        // For StringOrByteStringLists, check the underlying elements to avoid
        // forcing conversions of ByteStrings to Strings.
        // TODO: Could we just prohibit nulls in all protobuf lists and get rid of this? Is
        // if even possible to hit this condition as all protobuf methods check for null first,
        // right?
        List<?> lazyValues = ((LazyStringList) values).getUnderlyingElements();
        LazyStringList lazyList = (LazyStringList) list;
        ....

also, I have performed the following checks:

• adding a collection of elements with null

val names = Seq.range(0, 10).map(i => if (i == 3) null else i.toString * 1024)
val drop = proto.Drop.newBuilder().setInput(sql).addAllColumnNames(names.asJava).build()

[info] - truncate repeated strings with nulls *** FAILED *** (3 milliseconds)
[info]   java.lang.NullPointerException: Element at index 3 is null.
[info]   at com.google.protobuf.AbstractMessageLite$Builder.addAllCheckingNulls(AbstractMessageLite.java:359)
[info]   at com.google.protobuf.AbstractMessageLite$Builder.addAll(AbstractMessageLite.java:414)
[info]   at org.apache.spark.connect.proto.Drop$Builder.addAllColumnNames(Drop.java:1240)

• add a null element

val drop = proto.Drop.newBuilder().setInput(sql).addColumnNames(null).build()

[info] - truncate repeated strings with nulls *** FAILED *** (4 milliseconds)
[info]   java.lang.NullPointerException:
[info]   at org.apache.spark.connect.proto.Drop$Builder.addColumnNames(Drop.java:1221)

As you can see, under normal circumstances, it is impossible to add a null element to the repeated type. So personally, I don't think this null check is necessary, but I don't object to adding it.

[zhengruifeng]

thanks for checking this!

[zhengruifeng]

thank you guys, merged to master