Copilot commented on code in PR #445:
URL: https://github.com/apache/arrow-dotnet/pull/445#discussion_r4113546066
##########
src/Apache.Arrow.Operations/Shredding/ShredSchemaInferer.cs:
##########
@@ -53,6 +53,28 @@ public ShredSchema Infer(IEnumerable<VariantValue> values,
ShredOptions options
return BuildSchema(stats, totalCount, options, 0);
}
+ /// <summary>
+ /// Infers a shredding schema by analyzing the given nullable values.
+ /// <c>null</c> entries are SQL-NULL rows and are ignored; they carry
no type
+ /// information and do not count toward frequency thresholds.
+ /// </summary>
+ /// <param name="values">The variant values to analyze.</param>
+ /// <param name="options">Options controlling depth, frequency, and
type consistency thresholds.</param>
+ /// <returns>An inferred <see cref="ShredSchema"/>.</returns>
+ public ShredSchema Infer(IEnumerable<VariantValue?> values,
ShredOptions options = null)
Review Comment:
Adding this overload makes an existing call such as `new
ShredSchemaInferer().Infer(null)` ambiguous with the original overload, so
source that previously compiled (and reached the documented argument
validation) now fails to compile. Please avoid overloading only on
`VariantValue` versus `VariantValue?` here—use a distinct nullable-entry-point
name or otherwise preserve an unambiguous null call.
##########
src/Apache.Arrow.Operations/Shredding/VariantShredder.cs:
##########
@@ -66,6 +66,39 @@ public static (byte[] Metadata, IReadOnlyList<ShredResult>
Rows) Shred(
return (metadataBytes, results);
}
+ /// <summary>
+ /// Shreds a column of nullable variant values. A <c>null</c> entry is
a
+ /// SQL-NULL row, as opposed to <see cref="VariantValue.Null"/>, which
is a
+ /// present variant null. SQL-NULL rows produce a <c>null</c> entry in
the
+ /// returned rows, which <see
cref="ShreddedVariantArrayBuilder.Build"/> turns
+ /// into a null element of the resulting array.
+ /// </summary>
+ public static (byte[] Metadata, IReadOnlyList<ShredResult> Rows) Shred(
+ IEnumerable<VariantValue?> values,
+ ShredSchema schema)
Review Comment:
This overload also makes `VariantShredder.Shred(null, schema)` ambiguous
with the existing overload, so previously compiling null-validation calls
become source-incompatible. Please use a distinct nullable-entry-point name or
another API shape that keeps the null call unambiguous; the PR description
currently only documents this as a catch rather than resolving it.
##########
src/Apache.Arrow.Operations/Shredding/VariantShredder.cs:
##########
@@ -66,6 +66,39 @@ public static (byte[] Metadata, IReadOnlyList<ShredResult>
Rows) Shred(
return (metadataBytes, results);
}
+ /// <summary>
+ /// Shreds a column of nullable variant values. A <c>null</c> entry is
a
+ /// SQL-NULL row, as opposed to <see cref="VariantValue.Null"/>, which
is a
+ /// present variant null. SQL-NULL rows produce a <c>null</c> entry in
the
+ /// returned rows, which <see
cref="ShreddedVariantArrayBuilder.Build"/> turns
+ /// into a null element of the resulting array.
+ /// </summary>
+ public static (byte[] Metadata, IReadOnlyList<ShredResult> Rows) Shred(
+ IEnumerable<VariantValue?> values,
+ ShredSchema schema)
+ {
+ if (values == null) throw new
ArgumentNullException(nameof(values));
+ if (schema == null) throw new
ArgumentNullException(nameof(schema));
+
+ List<VariantValue?> rows = values as List<VariantValue?> ?? new
List<VariantValue?>(values);
+
+ VariantMetadataBuilder metadata = new VariantMetadataBuilder();
+ foreach (VariantValue? row in rows)
+ {
+ if (row.HasValue) CollectFieldNames(row.Value, metadata);
+ }
+ byte[] metadataBytes = metadata.Build(out int[] idRemap);
+
+ ShredResult[] results = new ShredResult[rows.Count];
+ for (int i = 0; i < rows.Count; i++)
+ {
+ VariantValue? row = rows[i];
+ results[i] = row.HasValue ? Shred(row.Value, schema, metadata,
idRemap) : null;
Review Comment:
This overload now returns a literal `null` `ShredResult` for SQL-NULL rows,
but `VariantUnshredder.Reconstruct` still rejects a null result with
`ArgumentNullException` before it can return its nullable result. Any caller
that round-trips the rows through the existing unshredder will therefore fail
on exactly the new case; update the reconstruction contract/implementation (and
its tests) to treat a null row as SQL NULL, or expose a non-null representation
consistently.
--
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]
