Gabriel39 commented on code in PR #66321:
URL: https://github.com/apache/doris/pull/66321#discussion_r3734664323
##########
be/src/core/data_type_serde/data_type_variant_v2_serde.cpp:
##########
@@ -175,6 +176,133 @@ void preflight_json(const IColumn& column, size_t start,
size_t end,
});
}
+void validate_paimon_variant_primitive(VariantPrimitiveId primitive_id) {
+ switch (primitive_id) {
+ case VariantPrimitiveId::NULL_VALUE:
+ case VariantPrimitiveId::TRUE_VALUE:
+ case VariantPrimitiveId::FALSE_VALUE:
+ case VariantPrimitiveId::INT8:
+ case VariantPrimitiveId::INT16:
+ case VariantPrimitiveId::INT32:
+ case VariantPrimitiveId::INT64:
+ case VariantPrimitiveId::DOUBLE:
+ case VariantPrimitiveId::DECIMAL4:
+ case VariantPrimitiveId::DECIMAL8:
+ case VariantPrimitiveId::DECIMAL16:
+ case VariantPrimitiveId::DATE:
+ case VariantPrimitiveId::TIMESTAMP_MICROS:
+ case VariantPrimitiveId::TIMESTAMP_NTZ_MICROS:
+ case VariantPrimitiveId::FLOAT:
+ case VariantPrimitiveId::BINARY:
+ case VariantPrimitiveId::STRING:
+ case VariantPrimitiveId::UUID:
+ return;
+ case VariantPrimitiveId::TIME_NTZ_MICROS:
+ case VariantPrimitiveId::TIMESTAMP_NANOS:
+ case VariantPrimitiveId::TIMESTAMP_NTZ_NANOS:
+ throw Exception(ErrorCode::NOT_IMPLEMENTED_ERROR,
+ "Paimon does not support Variant primitive id {}",
+ static_cast<uint8_t>(primitive_id));
+ }
+ throw Exception(ErrorCode::NOT_IMPLEMENTED_ERROR,
+ "Paimon does not support unknown Variant primitive id {}",
+ static_cast<uint8_t>(primitive_id));
+}
+
+void validate_paimon_variant_value(VariantRef value, uint32_t depth = 0) {
+ if (depth > VARIANT_MAX_NESTING_DEPTH) {
+ throw Exception(ErrorCode::CORRUPTION, "Variant value exceeds maximum
nesting depth {}",
+ VARIANT_MAX_NESTING_DEPTH);
+ }
+ const size_t encoded_size = value.value_size();
+ if (encoded_size != value.value.size) {
+ throw Exception(ErrorCode::CORRUPTION,
+ "Variant value has {} trailing bytes after the encoded
value",
+ value.value.size - encoded_size);
+ }
+
+ switch (value.basic_type()) {
+ case VariantBasicType::PRIMITIVE:
+ validate_paimon_variant_primitive(value.primitive_id());
+ return;
+ case VariantBasicType::SHORT_STRING:
+ return;
+ case VariantBasicType::OBJECT:
+ for (uint32_t i = 0; i < value.num_elements(); ++i) {
+ uint32_t field_id = 0;
+ VariantRef child = value.object_value_at(i, &field_id);
+ value.metadata.key_at(field_id);
+ validate_paimon_variant_value(child, depth + 1);
+ }
+ return;
+ case VariantBasicType::ARRAY:
+ for (uint32_t i = 0; i < value.num_elements(); ++i) {
+ validate_paimon_variant_value(value.array_at(i), depth + 1);
+ }
+ return;
+ }
+}
+
+void require_variant_arrow_status(const arrow::Status& status) {
+ if (!status.ok()) {
+ throw Exception(ErrorCode::INTERNAL_ERROR, "Variant V2 Arrow append
failed: {}",
+ status.ToString());
+ }
+}
+
+Status write_binary_variant_arrow(const IColumn& column, const NullMap*
null_map,
+ arrow::StructBuilder& builder, size_t start,
size_t end) {
+ // StructBuilder::type() returns a shared_ptr by value. Keep that owner
alive while using the
+ // cast reference; otherwise the reference would dangle as soon as the
temporary is destroyed.
+ const auto builder_type = builder.type();
+ const auto& struct_type = assert_cast<const
arrow::StructType&>(*builder_type);
+ if (struct_type.num_fields() != 2 || struct_type.field(0)->name() !=
"value" ||
+ struct_type.field(1)->name() != "metadata" ||
+ struct_type.field(0)->type()->id() != arrow::Type::BINARY ||
+ struct_type.field(1)->type()->id() != arrow::Type::BINARY) {
+ return Status::InvalidArgument(
+ "Binary Variant V2 Arrow type must be "
+ "struct<value: binary, metadata: binary>, got {}",
+ struct_type.ToString());
+ }
+ auto* value_builder =
dynamic_cast<arrow::BinaryBuilder*>(builder.field_builder(0));
+ auto* metadata_builder =
dynamic_cast<arrow::BinaryBuilder*>(builder.field_builder(1));
+ if (value_builder == nullptr || metadata_builder == nullptr) {
+ return Status::InvalidArgument("Binary Variant V2 Arrow child builders
must be binary");
+ }
+
+ // GenericVariant assumes its input is valid, and Paimon's unshredded
writer copies these two
+ // buffers without inspecting them. Validate once at the Doris-to-Paimon
boundary so a write
+ // cannot commit bytes which Paimon is unable to read later.
+ const auto outer_nulls = forced_nulls(null_map);
+ visit_variant_v2_values(
+ column, start, end, outer_nulls,
+ [&](size_t) { require_variant_arrow_status(builder.AppendNull());
},
+ [&](size_t row, VariantRef value) {
+ try {
+ constexpr size_t PAIMON_VARIANT_SIZE_LIMIT = 128 * 1024 *
1024;
+ if (value.value.size > PAIMON_VARIANT_SIZE_LIMIT ||
+ value.metadata.size > PAIMON_VARIANT_SIZE_LIMIT) {
+ throw Exception(ErrorCode::INVALID_ARGUMENT,
+ "exceeds the 128 MiB value/metadata
limit");
+ }
+ value.metadata.validate();
Review Comment:
[P2] Avoid per-row validation and copying of shared Variant metadata
`ColumnVariantV2` deduplicates metadata and lets many rows reference the
same metadata id, but this path validates that complete dictionary once per row
and then appends it once per row to Arrow. On the Java side,
`VarBinaryVector.get(index)` copies it again into a fresh `byte[]` for every
`GenericVariant`. A batch of sparse objects sharing a large dictionary
therefore performs O(rows × metadata_size) validation, native copying, IPC
transfer, and Java heap allocation, even when each row uses only a small part
of the dictionary.
This is related to the aggregate 2 GiB issue, but splitting the block only
bounds peak memory; it does not remove the total CPU, copy, and GC
amplification. Please validate each distinct metadata id once and avoid
materializing the same metadata bytes per row across the Arrow/JNI boundary
where possible. Add a stress or benchmark case with many rows sharing one large
metadata dictionary to measure throughput and allocation behavior.
--
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]
