github-actions[bot] commented on code in PR #64680:
URL: https://github.com/apache/doris/pull/64680#discussion_r3459161584
##########
fe/fe-type/src/main/java/org/apache/doris/catalog/VariantType.java:
##########
@@ -187,6 +187,11 @@ public String toSql(int depth) {
sb.append("\"variant_sparse_hash_shard_count\" = \"")
.append(String.valueOf(Math.max(1,
variantSparseHashShardCount))).append("\"");
}
+ if (enableNestedGroup) {
Review Comment:
When nested group is enabled the parser zeros
`variantMaxSparseColumnStatisticsSize` above, but this catalog `toSql()` path
still emits that value in the non-doc property set before adding
`variant_enable_nested_group`. A table created with
`VARIANT<PROPERTIES("variant_enable_nested_group" = "true")>` will therefore
print `variant_max_sparse_column_statistics_size = "0"`, and re-parsing that
SQL fails because
`PropertyAnalyzer.analyzeVariantMaxSparseColumnStatisticsSize` rejects explicit
values below 1. Please serialize a nested-group property set that omits the
disabled sparse/doc properties, or otherwise keeps all emitted values
parser-valid, and add a DDL round-trip test for this type.
##########
be/src/common/config.cpp:
##########
@@ -1172,7 +1172,11 @@ DEFINE_mBool(variant_enable_duplicate_json_path_check,
"false");
DEFINE_mInt32(variant_storage_parse_mode, "0");
DEFINE_mBool(enable_vertical_compact_variant_subcolumns, "true");
DEFINE_mBool(enable_variant_doc_sparse_write_subcolumns, "true");
-DEFINE_mBool(variant_nested_group_discard_scalar_on_conflict, "false");
+// Maximum depth of nested arrays to track with NestedGroup
+// Reserved for future use when NestedGroup expansion moves to storage layer
+// Deeper arrays will be stored as JSONB
+DEFINE_mInt32(variant_nested_group_max_depth, "10");
Review Comment:
This new config is defined and registered, but it is not declared in
`config.h` next to the other variant configs. `DECLARE_mInt32` is what exposes
`config::...` values to BE code outside `config.cpp`, so any code that tries to
use `config::variant_nested_group_max_depth` through the normal config header
will fail to compile even though the runtime config name exists. Please add the
matching `DECLARE_mInt32(variant_nested_group_max_depth);` near the adjacent
variant declarations.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -5329,8 +5329,11 @@ public DataType
visitVariantPredefinedFields(VariantPredefinedFieldsContext ctx)
}
if (enableNestedGroup) {
- throw new NotSupportedException(
- "variant_enable_nested_group is not supported now");
+ enableVariantDocMode = false;
Review Comment:
This now lets FE create a table whose VARIANT column has
`variant_enable_nested_group=true`, but the default BE build still cannot write
such a column. FE propagates the flag into tablet schema, and
`VariantColumnWriterImpl::finalize()` calls the nested-group provider whenever
the flag is true; the default provider's `prepare`/`prepare_with_built_groups`
implementations now return `NotSupported("NestedGroup write path is not
available in this build")`. That means a user can successfully create the table
and only hit an insert/load failure later, while the new BE tests skip this
path when the provider is unavailable. Please either keep DDL/session
acceptance gated until the active BE write path is available, provide a safe
fallback write behavior, or add an explicit negative end-to-end test that
documents this create-vs-insert boundary.
--
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]
