eldenmoon commented on code in PR #69761:
URL: https://github.com/apache/doris/pull/69761#discussion_r4431832547
##########
be/src/storage/tablet/tablet_schema.cpp:
##########
@@ -57,6 +57,7 @@
#include "storage/types.h"
#include "storage/utils.h"
#include "util/json/path_in_data.h"
+#include "util/string_util.h"
Review Comment:
Not a regression. On master, `TabletSchema::inverted_indexs(const
TabletColumn&)` already returns `{}` for a VARIANT root column:
`IndexColumnWriter::check_support_inverted_index()` rejects
`column.is_variant_type()` (`be/src/storage/index/index_writer.cpp:34`). So a
direct-root SEARCH bound to an ordinary parent index such as `idx_var2(v)` gets
no iterator on master either. This PR only adds values-scope indexes to that
result; tables without a values index see exactly master's behavior.
##########
be/src/storage/tablet/tablet_schema.cpp:
##########
@@ -1645,6 +1646,13 @@ std::vector<TabletIndexPtr>
TabletSchema::inverted_index_by_field_pattern(
}
std::vector<const TabletIndex*> TabletSchema::inverted_indexs(const
TabletColumn& col) const {
Review Comment:
Same reasoning: on master this overload returns `{}` for the VARIANT root,
so `IndexBuilder` never found an ordinary parent index through it. The PR only
adds values-scope indexes to the result, and FE rejects `DROP INDEX` for a
values index. Dropping an ordinary parent index behaves as on master.
##########
be/src/storage/segment/variant/v2/variant_column_writer.cpp:
##########
@@ -108,6 +115,9 @@ Status VariantV2ColumnWriter::init() {
Review Comment:
Stale at the current head. `VariantV2ColumnWriter::init()` returns `VARIANT
root-only writer cannot rebuild a values index`
(`variant_column_writer.cpp:127`) before any `ScalarColumnWriter` is built, so
the SNII factory is never asked to index `TYPE_VARIANT`. No supported path
reaches the guard: compaction keeps a root that must be rebuilt out of the
extracted-column expansion (`variant_util.cpp:1231`), and schema change, load,
MOW publish and the index builder write with schemas that carry no extracted
columns.
##########
be/src/exec/common/variant_util.cpp:
##########
@@ -852,6 +855,78 @@ Status
VariantCompactionUtil::aggregate_variant_extended_info(
return Status::OK();
}
+Status VariantCompactionUtil::root_index_plan_preserves_tokens(const
VariantExtendedInfo& info,
+ const
TabletSchema& plan,
+ int32_t
column_uid) {
+ const auto& parent = plan.column_by_uid(column_uid);
+ if (parent.variant_enable_nested_group()) {
+ return Status::NotSupported(
+ "root index native merge does not support nested-group
conversion");
+ }
+ // Sparse paths have no complete type metadata. A newly materialized
sparse path can
+ // change boolean/numeric or floating text; keep the whole value for the
rebuild writer.
+ if (!info.sparse_paths.empty() || !parent.get_sub_columns().empty()) {
+ return Status::NotSupported("root index conversion includes sparse or
declared paths");
+ }
+ // The VARIANT root may itself hold promoted scalar/array values, even
when there is
+ // no extracted column. Include that runtime merge in the proof as well.
+ if (!parent.variant_enable_doc_mode()) {
+ const auto root = info.path_to_data_types.find(PathInData());
+ if (root != info.path_to_data_types.end() && !root->second.empty()) {
+ const auto first = remove_nullable(root->second.front());
+ for (const auto& type : root->second) {
+ if (!first->equals(*remove_nullable(type))) {
+ return Status::NotSupported(
+ "root scalar conversion is not proven token
preserving");
+ }
+ }
+ }
+ }
+ for (const auto& column : plan.columns()) {
+ if (!column->is_extracted_column() || column->parent_unique_id() !=
column_uid ||
Review Comment:
This is a VARIANT V2 compaction issue that exists without any index.
Reproduced on master a72dcaa17b1 with a plain `VARIANT` column
(`variant_max_subcolumns_count=2`, no index):
```
R0: (1,'{"a":"needle","a.b":[]}'), (2,'{"a.b":[1]}') R1: 3 x
'{"c":1,"d":2}'
before full compaction: SELECT v -> 1 {"a":"needle"}
after full compaction: SELECT v -> 1 {"a":{"b":[]}}
```
Compaction itself changes the row's logical value (a materialized empty
array is ignored by the assembler, the same `[]` written as a sparse cell
becomes a visible descendant). The values index faithfully indexes the logical
value it was given; it cannot be correct while the stored value changes
underneath it. We will fix the layout rule separately so compaction preserves
the logical value, which also fixes the index.
##########
be/src/exec/common/variant_util.cpp:
##########
@@ -1152,19 +1152,50 @@ void
VariantCompactionUtil::get_compaction_subcolumns_from_data_types(
append_empty_key_subcolumn_from_stats(paths_set_info, parent_column,
output_schema);
}
+bool VariantCompactionUtil::has_values_index(const TabletSchema& schema) {
+ return std::ranges::any_of(schema.inverted_indexes(), [](const
TabletIndex* index) {
+ return index->is_variant_values_index();
+ });
+}
+
+Status VariantCompactionUtil::plan_values_index_compaction(
+ const std::vector<RowsetSharedPtr>& rowsets, bool expand_subcolumns,
+ const std::set<std::pair<int32_t, int64_t>>& native_indexes,
TabletSchemaSPtr& schema,
+ VariantCompactionPathsSPtr* paths) {
+ std::set<int32_t> rebuild_roots;
+ for (const auto* index : schema->inverted_indexes()) {
+ if (index->is_variant_values_index() &&
+ !native_indexes.contains({index->col_unique_ids().front(),
index->index_id()})) {
Review Comment:
Fixed in 7234519d70f. `has_values_index()` and
`plan_values_index_compaction()` skip a values index with empty column IDs, as
the existing index compaction does (`be/src/exec/common/variant_util.cpp:1161`,
`:1172`). UT:
`IndexStorageVariantCompactionUtilTest.ValuesIndexPlanningSkipsEmptyColumnIds`.
--
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]
