github-actions[bot] commented on code in PR #67983:
URL: https://github.com/apache/doris/pull/67983#discussion_r4043280447
##########
be/src/storage/segment/variant/v2/variant_path_builder.cpp:
##########
@@ -219,8 +219,16 @@ const DataTypePtr& cached_decimal_type(uint32_t scale) {
return types[scale];
}
-DataTypePtr infer_type(VariantRef value, const DataTypePtr& reusable_type =
nullptr) {
- const ValueKind kind = value_kind(value);
+const DataTypePtr& array_element_type(const DataTypeArray& array) {
+ // DataTypeArray always wraps its element in Nullable. Borrow the element
instead of copying it
+ // through remove_nullable(): element types are process-wide statics
shared by concurrent
+ // flushes, so each shared_ptr copy is a contended reference-count update.
+ return assert_cast<const
DataTypeNullable&>(*array.get_nested_type()).get_nested_type();
+}
+
+// Every scalar storage type is a process-wide static, so it is returned by
reference.
+const DataTypePtr& infer_scalar_type(VariantRef value, ValueKind kind) {
+ DORIS_CHECK(kind != ValueKind::ARRAY);
Review Comment:
[P2] Use a debug-only check for this established hot-path invariant
`infer_scalar_type()` is called once per scalar array element, while both
callers already establish `kind != ARRAY` (the top-level branch checks it and
the array loop returns before this call for nested arrays). `DORIS_CHECK` has
release semantics, even though inlining may let a particular optimized build
prove and remove it, and the repository guidance explicitly requires `DCHECK`
for invariants in performance-sensitive loops. Please use `DCHECK` here (or
remove the redundant check) so release behavior does not depend on the
optimizer eliminating a check from the exact loop this PR targets.
##########
be/test/storage/variant/variant_column_writer_reader_test.cpp:
##########
@@ -758,6 +759,69 @@ TEST(VariantPathBuilderTest,
PreservesIncomingArrayWhenInferredDecimalPromotionO
"[9999999999999999999999999999999999999.9]");
}
+TEST(VariantPathBuilderTest, ArrayPathReusesElementTypeAcrossRows) {
+ VariantBatchBuilder value_builder;
+ const auto append_array = [&](auto&& fill) {
+ auto row = value_builder.begin_row();
+ auto array = row.start_array();
+ fill(row);
+ array.finish();
+ row.finish();
+ };
+ append_array([](auto& row) {
+ row.add_float(1.0F);
+ row.add_float(2.0F);
+ });
+ append_array([](auto& row) {
+ row.add_float(3.0F);
+ row.add_null();
+ });
+ append_array([](auto& row) { row.add_null(); });
+ append_array([](auto& row) { row.add_double(4.5); });
+ append_array([](auto& row) { row.add_float(5.0F); });
+ VariantBatchBuilder values = value_builder.finish_batch();
+
+ const auto element_primitive = [](const DataTypePtr& type) {
+ const DataTypePtr array = remove_nullable(type);
+ return remove_nullable(assert_cast<const
DataTypeArray&>(*array).get_nested_type())
+ ->get_primitive_type();
+ };
+ segment_v2::VariantPathBuilder builder(PathInData("metric"));
+ // FLOAT arrays, arrays with null elements, and all-null arrays share the
first row's type.
+ for (size_t row = 0; row < 3; ++row) {
+ ASSERT_TRUE(builder.append(values.value_at(row), row).ok());
+ EXPECT_EQ(builder.promotion_count(), 0) << "row=" << row;
Review Comment:
[P2] Make this test observe the optimized path
`promotion_count()` only changes when `Impl::promote()` rewrites the stored
column. On the base code, same-type FLOAT arrays already returned
`reusable_type` after paying `path_least_common_type(Nullable(FLOAT), FLOAT)`,
so these rows still reported zero promotions; the DOUBLE row caused the same
one promotion, and every type/value/null assertion below was identical. The
per-element static `shared_ptr` copies are also invisible here. As a result,
this test stays green if both performance fixes are reverted. Please add a
deterministic BE_TEST counter/hook (or equivalent oracle) for the avoided
common-type/owning-copy path that fails on the base implementation, while
keeping these semantic assertions.
##########
be/src/storage/segment/variant/v2/variant_path_builder.cpp:
##########
@@ -219,8 +219,16 @@ const DataTypePtr& cached_decimal_type(uint32_t scale) {
return types[scale];
}
-DataTypePtr infer_type(VariantRef value, const DataTypePtr& reusable_type =
nullptr) {
- const ValueKind kind = value_kind(value);
+const DataTypePtr& array_element_type(const DataTypeArray& array) {
+ // DataTypeArray always wraps its element in Nullable. Borrow the element
instead of copying it
+ // through remove_nullable(): element types are process-wide statics
shared by concurrent
+ // flushes, so each shared_ptr copy is a contended reference-count update.
+ return assert_cast<const
DataTypeNullable&>(*array.get_nested_type()).get_nested_type();
Review Comment:
[P2] Use the typed accessor instead of release RTTI here
The default `assert_cast` has release `typeid`/failure semantics, and this
helper is reached from reusable inference, representability, and append for
repeated array values. `DataTypeArray` is `final`; its constructor already
validates and stores the wrapper as `DataTypeNullablePtr`, and
`get_nullable_nested_type()` exposes that owner directly. Please return
`array.get_nullable_nested_type()->get_nested_type()` so this borrow keeps the
same lifetime guarantee without making removal of redundant RTTI on the hot
path optimizer-dependent.
--
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]
