harshmotw-db commented on code in PR #59669:
URL: https://github.com/apache/spark/pull/59669#discussion_r4419444748
##########
common/variant/src/main/java/org/apache/spark/types/variant/VariantBuilder.java:
##########
@@ -205,6 +205,79 @@ public static Variant pickAtPaths(Variant v, PickNode
tree) {
return builder.result();
}
+ // Return v in canonical form: only referenced dictionary keys, unique and
sorted by unsigned
+ // UTF-8 bytes; object fields stored in key order; minimal integer, decimal,
and offset widths;
+ // strings of at most 63 UTF-8 bytes use short-string encoding; decimal
trailing zeros are
+ // stripped, and integral decimals become longs only when they fit.
Normalize floating-point
+ // -0.0 to 0.0 and NaNs within each type. Preserve string and key bytes,
including invalid UTF-8.
+ // Return v itself if it is already canonical.
+ public static Variant canonicalize(Variant v) {
+ if (isCanonical(v)) {
Review Comment:
When I was exploring this, I had ideas about having an additional field in
the `Variant` class saying `isCanonical` which doCanonicalize would mark as
`true`. This way, we would make sure that each variant is only get
canonicalized once. Checking `isCanonical` could potentially be expensive if
you plan on having this `canonicalize_variant` expression with every equality
operation. In these cases, rows would have `doCanonicalize` run once and
`isCanonical` run over and over again on the same variant that you already know
is canonicalized. `isCanonical` is probably not even asymptotically faster than
`doCanonicalize` (I guess it mainly would be avoiding many of the mempry
allocations and copying)
Another benefit of this would be that `isCanonical` is riskier than
`doCanonicalize`. You need to be far more careful when making sure something is
right because you might miss checking something. Think of cases where the
variant spec is modified in the future. The trivial implementation of
`doCanonicalize` would fail at anything it doesn't understand because it needs
to be able to parse and reconstruct everything. `isCanonical` would need to
intentionally be written in a way that it is tolerant of any changes.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
