github-actions[bot] commented on code in PR #69812:
URL: https://github.com/apache/doris/pull/69812#discussion_r4438622688
##########
be/src/storage/segment/variant/variant_column_reader.cpp:
##########
@@ -185,6 +185,27 @@ class ReaderOwnedColumnIterator final : public
ColumnIterator {
std::shared_ptr<ColumnReader> _owner;
};
+// V2 with count 0 keeps dynamic paths (and typed ones with
typed_paths_to_sparse) in the sparse
+// column, so a template path `t` may be a subcolumn but a dynamic `a` may not.
+void check_variant_serialize_type_layout(const TabletColumn& column,
+ const SubcolumnColumnMetaInfo&
subcolumns) {
+ if (column.variant_serialize_type() !=
VariantSerializeTypePB::VARIANT_SERIALIZE_V2 ||
+ column.variant_max_subcolumns_count() != 0 ||
column.variant_enable_doc_mode() ||
+ column.variant_enable_nested_group()) {
+ return;
+ }
+ for (const auto& entry : subcolumns) {
+ if (entry->path.empty()) {
+ continue; // the root
+ }
+ DORIS_CHECK(entry->path.get_is_typed() &&
!column.variant_enable_typed_paths_to_sparse())
Review Comment:
[P1] Accept historical layouts written by old cloud BEs
With the default `write_schema_kv=true`, upgrade the FE before all BEs,
create an ordinary count-zero VARIANT column, and load `{"a":1}` through an old
BE. That BE ignores the new policy field and materializes `a`. However,
`CloudInternalCatalog` has already persisted the V2 root schema;
`commit_rowset` separates dynamic leaves into the dictionary while
`put_schema_kv` retains that existing root. `get_rowset` therefore reconstructs
a V2/count-zero schema for this materialized segment. A normal read after
syncing it onto a fresh/upgraded BE reaches this check and fails (exception in
Release, abort in Debug), making committed rows unreadable.
This is distinct from [existing compaction
finding](https://github.com/apache/doris/pull/69812#discussion_r4410002990): it
affects ordinary cloud reads without compaction, changelog, or a later schema
change. Preserve/recognize the producer's physical layout, or gate V2 creation
until writers support it, and cover an old-BE write followed by an upgraded-BE
cloud read.
##########
fe/fe-type/src/main/java/org/apache/doris/catalog/VariantType.java:
##########
@@ -151,6 +162,38 @@ public VariantType(ArrayList<VariantField> fields, int
variantMaxSubcolumnsCount
this.variantDocMaterializationMinRows =
variantDocMaterializationMinRows;
this.variantDocShardCount = variantDocShardCount;
this.enableNestedGroup = enableNestedGroup;
+ this.variantSerializeType = variantSerializeType;
+ }
+
+ /** This type with the given serialize type, as a new instance. */
+ public VariantType withSerializeType(TVariantSerializeType serializeType) {
+ return new VariantType(predefinedFields, variantMaxSubcolumnsCount,
enableTypedPathsToSparse,
+ variantMaxSparseColumnStatisticsSize,
variantSparseHashShardCount, enableVariantDocMode,
+ variantDocMaterializationMinRows, variantDocShardCount,
enableNestedGroup, serializeType);
+ }
+
+ /**
+ * The layout a changelog stores this column with, as a new instance; this
one is untouched. Both images of a
+ * changelog row are only read whole, so every path goes to the sparse
column (V2 with no dynamic subcolumn and
+ * typed paths sent to the sparse column) whatever this column's layout
is. The predefined fields stay, so typed
+ * paths keep their types.
+ */
+ public VariantType forChangelog() {
+ Preconditions.checkState(getClass() == VariantType.class,
+ "forChangelog copies a plain VariantType, not %s",
getClass().getName());
+ // doc mode and nested group clear the sparse statistics size, so use
its default
+ int sparseColumnStatisticsSize = variantMaxSparseColumnStatisticsSize
> 0
+ ? variantMaxSparseColumnStatisticsSize : 10000;
+ return new VariantType(predefinedFields,
Review Comment:
[P1] Preserve typed doc snapshots when deriving changelog images
Retaining `predefinedFields` while clearing doc mode changes whole document
values. A supported source `VARIANT<'a':INT,
PROPERTIES("variant_enable_doc_mode"="true",
"variant_doc_materialization_min_rows"="100")>` with changelog enabled can
store `{"a":"1","keep":1}` in a one-row load. `finish_doc` publishes the raw
doc map before typed conversion, and the whole-root DOC reader does not overlay
materialized typed leaves, so `SELECT v FROM t` returns that original string.
The image instead takes `finish_ordinary`, which converts all typed paths
before sparse publication: its AFTER value becomes `{"a":1,"keep":1}`. An
uncastable string such as `"x"` loses `a` entirely. Historical BEFORE values
read from the source are recast the same way.
Preserve the source's whole doc snapshot when deriving its image instead of
applying ordinary typed-storage conversion. Add source/AFTER/BEFORE comparisons
with coercible and invalid strings; the current tests cover typed ordinary
columns and untyped doc columns separately.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/VariantSubPathPruning.java:
##########
@@ -91,6 +94,17 @@ public class VariantSubPathPruning implements CustomRewriter
{
@Override
public Plan rewriteRoot(Plan plan, JobContext jobContext) {
+ // A changelog stores its Variant columns with every path in the
sparse column and maps a BEFORE image onto
+ // its AFTER column by whole column, so a changelog scan reads Variant
columns whole and sub-paths are taken
+ // above it. Its plan may union the changelog scan with a base table
scan, whose union pushdown needs every
+ // child to supply the same sub-path slots, so the whole statement
keeps whole-column reads. changelog() is
+ // still a TVF relation here: it only gets its changelog scan during
physical translation.
+ if (plan.anyMatch(node -> (node instanceof LogicalOlapScan
+ && ((LogicalOlapScan) node).getTable() instanceof
OlapChangelogTableWrapper)
+ || (node instanceof LogicalTVFRelation
+ && ((LogicalTVFRelation) node).getFunction() instanceof
Changelog))) {
+ return plan;
Review Comment:
[P2] Keep required SEARCH bindings on independent base scans
The statement-wide return also skips extraction required by an unrelated
indexed scan. With a stored/indexed `v.a` on `indexed_base` and a populated
changelog-enabled `ct`, use `SELECT k FROM indexed_base WHERE
search('v.a:hello') UNION ALL SELECT k FROM changelog('table'='ct')` (both `k`
columns INT). The reduced plan before this rule is:
```text
UnionAll(k)
Project(k)
Filter(SearchExpression(element_at(v, 'a')))
OlapScan(indexed_base)
Project(k)
TVF(changelog(ct))
```
`RewriteSearchToSlots` creates that ElementAt expecting this rule to replace
it with the stored subpath slot. This guard leaves it intact; BE
`collect_search_inputs` treats it as unmaterialized, and
`verify_missing_variant_path` rejects the existing `a` with
`INVERTED_INDEX_NOT_SUPPORTED`. SEARCH has no row fallback, so the query fails
despite a usable index. Keep the required ordinary-scan bindings while
retaining whole-value changelog reads. This scalar-only UNION is distinct from
the already-reported VARIANT union-output arity issue.
--
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]
