CurtHagenlocher commented on code in PR #445:
URL: https://github.com/apache/arrow-dotnet/pull/445#discussion_r4113950332
##########
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:
Keeping the overload. The only calls that become ambiguous are an untyped
`null` or `default` literal for `values`, and those calls could only ever throw
`ArgumentNullException`, so nothing that does useful work is affected. A null
in a typed variable, arrays and collection expressions (including `[v, null]`)
all still resolve correctly, and already-compiled callers stay bound to the
original method. `VariantArray.Builder.AppendRange` already uses the same
`VariantValue` / `VariantValue?` overload pair, so a separate name would be
less consistent with the rest of the library. I've updated the PR description
to explain this.
##########
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:
Keeping the overload. The only calls that become ambiguous are an untyped
`null` or `default` literal for `values`, and those calls could only ever throw
`ArgumentNullException`, so nothing that does useful work is affected. A null
in a typed variable, arrays and collection expressions (including `[v, null]`)
all still resolve correctly, and already-compiled callers stay bound to the
original method. `VariantArray.Builder.AppendRange` already uses the same
`VariantValue` / `VariantValue?` overload pair, so a separate name would be
less consistent with the rest of the library. I've updated the PR description
to explain this.
##########
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:
Good catch, fixed in 9155656. `Reconstruct` now returns `null` for a `null`
result instead of throwing. That matches its existing contract, where `null`
already means "no value". A top-level row can't legitimately be missing, so at
the top level `null` can only mean a SQL-NULL row. Added
`Reconstruct_RoundTripsNullRows`, which shreds rows containing both a SQL-NULL
row and a present `VariantValue.Null` and checks that each reconstructs
correctly.
--
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]
