voonhous commented on code in PR #19583:
URL: https://github.com/apache/hudi/pull/19583#discussion_r3772837826
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/MergeOnReadSnapshotRelation.scala:
##########
@@ -132,7 +133,15 @@ abstract class BaseMergeOnReadSnapshotRelation(sqlContext:
SQLContext,
getPartitionColumnsAsInternalRow(file.getPathInfo),
file.getPathInfo.getPath, 0, file.getFileSize)
}
- HoodieMergeOnReadFileSplit(partitionedBaseFile, logFiles)
+ // These values are empty unless the partition columns are omitted from
the data files, which is
Review Comment:
Right, the extract-from-path read option applies to tables that do persist
their partition columns, so "empty unless the columns are omitted" was wrong.
Reworded to name `shouldExtractPartitionValuesFromPartitionPath`'s three
triggers instead: dropped partition columns, the extract-from-path read option,
or a bootstrap data-queries-only read. Fixed in b78ba3df.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/cdc/InternalRowToJsonStringConverter.scala:
##########
@@ -90,10 +91,40 @@ class InternalRowToJsonStringConverter(schema: StructType) {
structMap.toMap
case _ => value // fallback
}
+ case dt if dt.typeName ==
InternalRowToJsonStringConverter.VARIANT_TYPE_NAME =>
+ // VariantVal.toString renders the variant as JSON; embed it as a
real JSON node so
+ // the image carries the variant's structure. Falling through to the
default would
+ // serialize the VariantVal bean, i.e. its raw value/metadata bytes
as base64.
+ // Matched on the type name rather than SparkAdapter.isVariantType:
this guard is
+ // evaluated for every non-string/array/map/struct field, and
resolving the adapter
+ // needs a version module that is not on hudi-spark-common's own
test classpath.
+ val variantJson = value.toString
+ try {
+ mapper.readTree(variantJson)
+ } catch {
+ // A variant can hold a field name, string or nesting depth past
Jackson's default
+ // StreamReadConstraints (50k chars, 20M chars, 1000 levels) while
staying well inside
+ // the variant size limit, and all three arrive here as
StreamConstraintsException. A
+ // CDC image is diagnostic data rather than the table's data, so
keep the rendering as
+ // a plain string instead of failing the query over it.
+ // NOTE: value.toString is deliberately outside this block. It
throws MALFORMED_VARIANT
+ // on corrupt bytes, which is a data-integrity problem an operator
has to see, not a
+ // rendering quirk to paper over -- and there would be no
rendering left to fall back to.
+ case _: JsonProcessingException => variantJson
Review Comment:
Confirmed and taken, not deferred -- it is a couple of lines and the case is
real. `RawValue` it is, in b78ba3df.
While confirming it I measured the window, and it is a little different from
999-1000: jackson-core's `WriterBasedJsonGenerator.writeStartObject(Object)`
validates the *parent* context's depth rather than the child's, so the object
path tolerates one extra level. That works out to a window of one level per
enclosing object for an object-rendered variant, and one more for an
array-rendered one. A variant at the top of the image rendering as an array
fails at exactly depth 1000, which is what the new test pins -- verified red
before the change with `StreamWriteConstraints.getMaxNestingDepth()` exceeded,
green after. Same behaviour on jackson 2.18.2, 2.20.0 and 2.21.2.
Two things the verbatim embed needed that the parsed-node path did not,
since `readTree` is a weaker gate than it looks: `FAIL_ON_TRAILING_TOKENS` on
the mapper, because it otherwise parses a valid prefix and leaves the rest
unconsumed, and an explicit `MissingNode` check, because blank input comes back
as `MissingNode` rather than throwing and `RawValue` would emit nothing at all.
Both would have spliced corrupt text into the image. Neither is producible by
`Variant.toJson`, which always emits exactly one complete value, so they are
defence in depth.
For the common case it is a byte-identical swap -- I checked the parsed-node
and raw paths against the real renderer across decimals, doubles, escaping, key
ordering and every scalar shape -- and it drops a parse-then-rebuild round
trip. One consequence worth naming: the image can now be written at a depth
above the default *read* limit, so a consumer re-parsing it with stock
constraints could reject it. That band exists today too (the node path can emit
1001); this widens it slightly, and for diagnostic data a deep-but-valid image
still beats failing the query.
--
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]
