github-actions[bot] commented on code in PR #69812:
URL: https://github.com/apache/doris/pull/69812#discussion_r4410002990
##########
be/src/exec/common/variant_util.cpp:
##########
@@ -1233,8 +1234,25 @@ Status
VariantCompactionUtil::get_extended_compaction_schema(
output_schema,
paths_set_info));
// 3. get the subpaths
- get_subpaths(column->variant_max_subcolumns_count(),
extended_info.path_to_none_null_values,
- paths_set_info);
+ // A count of 0 means no limit under V1 and no dynamic subcolumn under
V2, whose paths all
+ // stay in the sparse column(s).
+ if (column->variant_max_subcolumns_count() > 0 ||
+ column->variant_serialize_type() ==
VariantSerializeTypePB::VARIANT_SERIALIZE_V1) {
+ get_subpaths(column->variant_max_subcolumns_count(),
+ extended_info.path_to_none_null_values,
paths_set_info);
+ } else {
+ // Every input path of a V2 count-0 column is in the sparse
column(s) or typed. Nothing
+ // would carry a materialized dynamic subcolumn into the output,
so its data would be lost.
+ for (const auto& [path, _] :
extended_info.path_to_none_null_values) {
+ if (!extended_info.sparse_paths.contains(path) &&
Review Comment:
[P1] Check materialized paths per input before accepting this layout
`extended_info.sparse_paths` is the union across every input segment, so an
older segment materializing `a` passes this guard whenever another segment
stores `a` sparsely. This is reachable during a rolling upgrade: the new FE
sends V2/count-zero changelog columns to an old BE, which ignores the new field
and materializes their paths; after upgrading that BE, a light ADD COLUMN and
another load produce a higher-schema-version V2 sparse rowset. Compaction
selects that V2 schema, but this branch leaves `paths_set_info.sparse_path_set`
empty. `_create_sparse_merge_reader` therefore skips the older materialized
`a`, while the root-only reader/writer cannot recover it (object roots are
empty). Old changelog images silently lose `a`, and `check_path_stats` now
skips changelog tablets. Populate the sparse transfer set for materialized
input leaves, or reject them using per-input materialized metadata rather than
membership in the union of sparse paths.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java:
##########
@@ -3086,8 +3087,13 @@ private boolean createOlapTable(Database db,
CreateTableInfo createTableInfo) th
for (Column column : baseSchema) {
if (column.isAutoInc()) {
throw new AnalysisException("auto-inc column can't be
created on table with changelog");
- } else if (column.getDataType().isVariantType()) {
- throw new AnalysisException("variant column can't be
created on table with changelog");
+ } else if (column.getType().isVariantType() &&
column.getVariantEnableNestedGroup()) {
+ throw new AnalysisException(
+ "variant column with nested group can't be
created on table with changelog");
+ } else if (column.getType().isVariantType()
Review Comment:
[P2] Update the cloud Variant-count invariant for BEFORE images
The newly accepted cloud MOW configuration with one VARIANT column and
`changelog.need_historical_value=true` has one source VARIANT but two changelog
VARIANT roots (AFTER and BEFORE). `CloudMetaMgr::update_tmp_rowset` still
asserts that those counts are equal at `cloud_meta_mgr.cpp:1697-1698`. This is
reachable in DEBUG/ASAN builds: let two same-key UPSERTs flush against the same
visible snapshot, then publish one before the other. The second publish's
`BaseTablet::update_delete_bitmap` creates a transient group writer to rebuild
the historical image, emits replacement rows, and reaches
`CloudTablet::save_delete_bitmap` with nonzero writer rows; that calls
`update_tmp_rowset` and aborts the BE on `DCHECK_EQ(1, 2)`. RELEASE builds
compile out this assertion, but assertion-enabled cloud builds cannot handle
this supported write schedule. Make the cloud invariant account for the BEFORE
columns and cover concurrent publish-time rewrites with a historical VARIANT
column.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/VariantSubPathPruning.java:
##########
@@ -91,6 +92,14 @@ 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.
+ if (plan.anyMatch(node -> node instanceof LogicalOlapScan
Review Comment:
[P2] Include changelog TVFs in the whole-column guard
`changelog(...)` remains a `LogicalTVFRelation` during this rewrite;
`TableChangelogFunction.getScanNode` attaches the wrapper only during physical
translation, so this predicate misses it. For a populated changelog-enabled
table `t` (with an inserted object containing `a`) and nullable `v
VARIANT<PROPERTIES("variant_max_subcolumns_count"="0","variant_enable_typed_paths_to_sparse"="true")>`,
`SELECT v['a'] FROM (SELECT v FROM t UNION ALL SELECT v FROM
changelog("table"="t")) u` has matching source/image types and no union casts.
Its reduced plan is:
```text
Project(element_at(v_union, 'a'))
UnionAll
OlapScan(t.v)
TVF(changelog.v)
```
`visitLogicalOlapScan` generates `v.a` only for the first child; there is no
TVF replacer. `visitLogicalUnion` then indexes `entry.getValue().get(1)`
although the list contains one slot, failing planning (reversing branches hits
the missing-subpath exception). `PushProjectThroughUnion` rejects ElementAt, so
it does not eliminate this shape. Recognize changelog TVF relations here too,
or synthesize the missing child expressions, and cover this matching-layout
UNION case.
--
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]
