github-actions[bot] commented on code in PR #69761:
URL: https://github.com/apache/doris/pull/69761#discussion_r4400052215
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/ExpressionAnalyzer.java:
##########
@@ -1019,9 +1020,14 @@ public Expression visitMatch(Match match,
ExpressionRewriteContext context) {
right.toSql(), match.toSql(), right.getDataType()));
}
- if (left.getDataType().isVariantType()) {
- left = new Cast(left, right.getDataType());
+ // Only direct roots use leaf-token semantics. Ordinary object
subpaths keep their
+ // existing JSON-to-STRING scalar semantics, including when pruning is
disabled.
+ if (left.getDataType().isVariantType()
+ && (!(left instanceof SlotReference) || ((SlotReference)
left).hasSubColPath()
Review Comment:
[P1] Resolve subpath aliases before choosing root MATCH semantics
A bare VARIANT slot is not sufficient evidence of a direct root. For a table
with an English values index and an ordinary `parser=none` path index, consider
this analyzed plan:
```text
Project(o#2 MATCH_ANY 'hello')
Project(v#1['obj'] AS o#2)
Scan(t.v#1)
```
`Alias.toSlot()` gives this derived `o#2` no subpath, so this condition
omits its STRING cast. `MergeProjectable`/`PlanUtils.mergeProjections` then
substitutes `ElementAt(v#1,'obj')` without rerunning coercion. With
nested-column pruning and index queries disabled, the translator sees the input
root slot `v#1`, binds its English values index, and BE recursively matches the
object's leaves. For `v={"obj":{"s":"hello"}}`, the derived query returns true,
while direct `v['obj'] MATCH_ANY 'hello'` receives the required STRING cast and
keyword JSON semantics, returning false. Preserve verified root/subpath lineage
across aliases, or perform this classification after resolving them.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/CheckMatchExpression.java:
##########
@@ -56,9 +57,22 @@ private Plan checkChildren(LogicalFilter<? extends Plan>
filter) {
+ " right operand is Literal. But meet expression %s",
matchExpression));
}
if (slotReference.getDataType().isVariantType() &&
!slotReference.hasSubColPath()) {
- throw new AnalysisException(String.format("VARIANT root column
does not support MATCH predicates. "
- + "Please query a subcolumn instead, for
example %s['field'] MATCH 'xxx'",
- slotReference.getName()));
+ if (matchExpression.getAnalyzer().isPresent()
+ &&
!"english".equals(matchExpression.getAnalyzer().get())) {
Review Comment:
[P2] Resolve analyzer names before enforcing English-only MATCH
On a valid English values-indexed table, `SELECT id FROM t WHERE v MATCH_ANY
'hello' USING ANALYZER ENGLISH` is rejected here. The parser preserves the
identifier spelling and `Match` only trims it, so this comparison sees
`ENGLISH`. The normal binding path later calls
`InvertedIndexUtil.resolveAnalyzerName`, which resolves this built-in to
`english` case-insensitively when no legacy custom policy shadows that exact
name; a root projection with the same spelling therefore succeeds. Apply the
same canonical resolution before enforcing the restriction and test
uppercase/mixed-case built-in names in filters as well as projections.
##########
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:
[P1] Prove visibility when materialized paths move into sparse storage
Checking only the output extracted-column types misses a layout change that
alters the logical leaves. With ordinary storage and
`variant_max_subcolumns_count=2`, let R0 contain `{"a":"needle","a.b":[]}` and
`{"a.b":[1]}`. Both paths are materialized; the assembler ignores materialized
empty arrays, so row 0 exposes `a="needle"` and its source index correctly
contains `needle`. Let R1 contain at least three rows `{"c":1,"d":2}`. All
sources have no sparse paths, but the final top-two plan extracts `c,d` and
moves `a,a.b` into sparse storage. This proof checks `c,d`, skips the sparse
map and permits native merge. `SparseColumnMergeIterator` serializes the
non-null `[]`; as a sparse cell it becomes a visible descendant and suppresses
`a`, leaving `{"a":{"b":[]}}` while the merged index still matches `needle`.
Row counts and path statistics remain equal, so publication checks pass.
Reject/rebuild such transitions or preserve their logical visibility. This
starts with valid source pos
tings and is separate from indexing already-hidden ancestors.
##########
be/src/storage/segment/variant/v2/variant_shredder.cpp:
##########
@@ -613,6 +617,88 @@ struct VariantShredder::Impl {
return publish_materialized(selection, candidate_builders, result);
}
+ // Typed paths are indexed only after the same conversion used by storage.
Merge compact
+ // rowids instead of scanning every path for every row or reconstructing
complete objects.
+ //
NOLINTNEXTLINE(readability-function-size,readability-function-cognitive-complexity,readability-make-member-function-const):
keep encoded leaf ownership and synchronous writer consumption in one row
scope; the root index writers are mutated.
+ Status write_converted_root_indexes(const VariantShreddedColumns& result,
+ const
DorisVector<VariantPathBuilder*>& builders) {
+ using namespace variant_v2::variant_assembler_detail;
+ using Position = std::pair<size_t, size_t>;
+ std::priority_queue<Position, DorisVector<Position>, std::greater<>>
pending;
+ DorisVector<PreparedMaterializedColumn> values;
+ DorisVector<size_t> positions(builders.size(), 0);
+ values.reserve(builders.size());
+ for (size_t index = 0; index < builders.size() &&
+ options.physical_layout !=
VariantShredderPhysicalLayout::DOC;
+ ++index) {
+ const auto* builder = builders[index];
+ if (builder->rowids().empty()) {
+ values.emplace_back();
+ continue;
+ }
+ values.push_back(prepare_materialized_column(builder->type(),
builder->column().get(),
+
builder->rowids().size()));
+ pending.emplace(builder->rowids().front(), index);
+ }
+ const auto& roots = assert_cast<const
ColumnString&>(*result.root_jsonb);
+ DorisVector<size_t> row_paths;
+ row_paths.reserve(builders.size() + 1);
+ for (size_t row = 0; row < rows; ++row) {
+ row_paths.clear();
+ VariantBatchBuilder encoded;
+ if (!index_nulls[row] && !roots.get_data_at(row).empty()) {
+ auto output = encoded.begin_row();
+ jsonb_to_variant(roots.get_data_at(row), output, 0);
+ output.finish();
+ row_paths.push_back(builders.size());
+ }
+ if (!index_nulls[row] &&
+ options.physical_layout == VariantShredderPhysicalLayout::DOC)
{
+ // DOC root reads the finalized binary buckets, not converted
typed sidecars.
+ for (const auto& bucket : result.binary_buckets) {
+ const auto& map = assert_cast<const
ColumnMap&>(*bucket.column);
+ const auto& cells = assert_cast<const
ColumnString&>(map.get_values());
+ const auto& offsets = map.get_offsets();
+ const size_t begin = row == 0 ? 0 : offsets[row - 1];
+ for (size_t cell = begin; cell < offsets[row]; ++cell) {
+ auto output = encoded.begin_row();
+
RETURN_IF_ERROR(variant_v2::append_v1_storage_cell(cells.get_data_at(cell),
+
output, 0));
+ output.finish();
+ row_paths.push_back(builders.size());
+ }
+ }
+ }
+ while (!pending.empty() && pending.top().first == row) {
+ const size_t index = pending.top().second;
+ pending.pop();
+ const size_t position = positions[index]++;
+ DCHECK(!index_nulls[row]);
+ auto output = encoded.begin_row();
+ RETURN_IF_ERROR(append_materialized_value(values[index],
position, output, 0));
Review Comment:
[P2] Handle declared Decimal256 paths before accepting the values index
With `enable_decimal256=true` and ordinary storage, CREATE TABLE accepts `v
VARIANT<'wide':DECIMAL(76,2)>` together with an English values index. Inserting
`parse_to_variant('{"wide":"123.45","text":"hello"}')` then converts the
declared builder to Decimal256 and fails at this new mandatory call:
`append_materialized_value` explicitly returns NotSupported for
TYPE_DECIMAL256. Existing `variant_decimal256_storage_error.groovy` verifies
that these declared schemas can INSERT successfully without a values index. Its
full-root SELECT is deliberately unsupported already; this is a new ingestion
failure after successful DDL, rather than an old MATCH regression. Handle these
no-token leaf/storage types without breaking ingestion, or reject the
incompatible values-index schema during DDL instead of accepting a table whose
legal inserts fail.
##########
be/src/storage/segment/variant/v2/variant_shredder.cpp:
##########
@@ -613,6 +617,88 @@ struct VariantShredder::Impl {
return publish_materialized(selection, candidate_builders, result);
}
+ // Typed paths are indexed only after the same conversion used by storage.
Merge compact
+ // rowids instead of scanning every path for every row or reconstructing
complete objects.
+ //
NOLINTNEXTLINE(readability-function-size,readability-function-cognitive-complexity,readability-make-member-function-const):
keep encoded leaf ownership and synchronous writer consumption in one row
scope; the root index writers are mutated.
+ Status write_converted_root_indexes(const VariantShreddedColumns& result,
+ const
DorisVector<VariantPathBuilder*>& builders) {
+ using namespace variant_v2::variant_assembler_detail;
+ using Position = std::pair<size_t, size_t>;
+ std::priority_queue<Position, DorisVector<Position>, std::greater<>>
pending;
+ DorisVector<PreparedMaterializedColumn> values;
+ DorisVector<size_t> positions(builders.size(), 0);
+ values.reserve(builders.size());
+ for (size_t index = 0; index < builders.size() &&
+ options.physical_layout !=
VariantShredderPhysicalLayout::DOC;
+ ++index) {
+ const auto* builder = builders[index];
+ if (builder->rowids().empty()) {
+ values.emplace_back();
+ continue;
+ }
+ values.push_back(prepare_materialized_column(builder->type(),
builder->column().get(),
+
builder->rowids().size()));
+ pending.emplace(builder->rowids().front(), index);
+ }
+ const auto& roots = assert_cast<const
ColumnString&>(*result.root_jsonb);
+ DorisVector<size_t> row_paths;
+ row_paths.reserve(builders.size() + 1);
+ for (size_t row = 0; row < rows; ++row) {
+ row_paths.clear();
+ VariantBatchBuilder encoded;
+ if (!index_nulls[row] && !roots.get_data_at(row).empty()) {
+ auto output = encoded.begin_row();
+ jsonb_to_variant(roots.get_data_at(row), output, 0);
+ output.finish();
+ row_paths.push_back(builders.size());
+ }
+ if (!index_nulls[row] &&
+ options.physical_layout == VariantShredderPhysicalLayout::DOC)
{
+ // DOC root reads the finalized binary buckets, not converted
typed sidecars.
+ for (const auto& bucket : result.binary_buckets) {
+ const auto& map = assert_cast<const
ColumnMap&>(*bucket.column);
+ const auto& cells = assert_cast<const
ColumnString&>(map.get_values());
+ const auto& offsets = map.get_offsets();
+ const size_t begin = row == 0 ? 0 : offsets[row - 1];
+ for (size_t cell = begin; cell < offsets[row]; ++cell) {
+ auto output = encoded.begin_row();
+
RETURN_IF_ERROR(variant_v2::append_v1_storage_cell(cells.get_data_at(cell),
+
output, 0));
+ output.finish();
+ row_paths.push_back(builders.size());
+ }
+ }
+ }
+ while (!pending.empty() && pending.top().first == row) {
+ const size_t index = pending.top().second;
+ pending.pop();
+ const size_t position = positions[index]++;
+ DCHECK(!index_nulls[row]);
+ auto output = encoded.begin_row();
+ RETURN_IF_ERROR(append_materialized_value(values[index],
position, output, 0));
+ output.finish();
+ row_paths.push_back(index);
+ if (positions[index] < builders[index]->rowids().size()) {
+
pending.emplace(builders[index]->rowids()[positions[index]], index);
+ }
+ }
+ // Root token values borrow bytes until end_document; retain all
encoded leaves
+ // for this row through synchronous SNII consumption.
+ auto batch = encoded.finish_batch();
+ for (auto* writer : options.root_index_writers) {
+ RETURN_IF_ERROR(writer->begin_document(index_nulls[row] != 0));
+ }
+ for (size_t leaf = 0; leaf < row_paths.size(); ++leaf) {
+
RETURN_IF_ERROR(append_variant_root_index_leaves(options.root_index_writers,
Review Comment:
[P1] Index only paths visible in the final logical row
For an ordinary values-indexed column, `{"a":"hidden","a.b":"visible"}`
creates distinct `a` and `a.b` builders, and this loop indexes both. Full-root
reads instead call `VariantAssembler::append_visible_merge_values`, which
suppresses `a` in favor of its descendant and reconstructs
`{"a":{"b":"visible"}}`. Consequently, `WHERE v MATCH_ANY 'hidden'` returns
this row with index queries enabled and excludes it on the scalar path; `NOT`
can lose the row instead. The DOC cell loop has the same problem. Apply the
assembler's final-row visibility rules before tokenizing, and cover
materialized/sparse and DOC ancestor collisions with index/scan parity cases.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/InvertedIndexUtil.java:
##########
@@ -504,9 +519,18 @@ public static boolean
canHaveMultipleInvertedIndexes(DataType colType, List<Inde
return false;
}
+ if
(indexDefs.stream().filter(IndexDefinition::isVariantRootIndex).count() > 1) {
+ return false;
+ }
+
Set<String> analyzerKeys = new HashSet<>();
Set<String> analyzerSelectors = new HashSet<>();
for (IndexDefinition indexDef : indexDefs) {
+ // Root and path indexes bind disjoint predicates. Only path
indexes share
+ // analyzer identities/selectors; the single-root constraint is
checked above.
+ if (indexDef.isVariantRootIndex()) {
Review Comment:
[P2] Apply root/path analyzer separation to ordinary index additions
This exemption only reaches CREATE TABLE. After creating `t` with an English
values index, `ALTER TABLE t ADD INDEX path_idx(v) USING INVERTED
PROPERTIES("parser"="english")` still reaches
`SchemaChangeHandler.checkDuplicateIndexes`, which compares against the root
index's analyzer selector and throws that the same selector already exists.
Declaring these two indexes together in CREATE TABLE succeeds. The added index
here is an ordinary path index, so the restriction on adding a values index
does not apply. Use the same root/path scope distinction in the ALTER duplicate
checker and cover this addition path.
--
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]
