Copilot commented on code in PR #64680:
URL: https://github.com/apache/doris/pull/64680#discussion_r3450648366
##########
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");
+DEFINE_mBool(variant_nested_group_discard_scalar_on_conflict, "true");
Review Comment:
A new integer config (`variant_nested_group_max_depth`) is introduced
without any validation, while nearby configs do use validators. Add a validator
to enforce a sensible range (e.g., > 0, and possibly an upper bound) to avoid
invalid depths being accepted at runtime.
##########
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;
+ variantMaxSubcolumnsCount = 0;
+ enableTypedPathsToSparse = false;
+ variantMaxSparseColumnStatisticsSize = 0;
+ variantSparseHashShardCount = 0;
}
Review Comment:
This unconditionally forces `enableVariantDocMode = false` when
`enableNestedGroup` is true, which prevents the parser from detecting/throwing
on an explicit configuration conflict (both `variant_enable_nested_group` and
`variant_enable_doc_mode` set to true). Given the added test asserting a
NotSupportedException for that conflict, the conflict check should happen
before mutating these flags (e.g., explicitly throw when both are true), and
only apply the overrides when NestedGroup is enabled without an explicit
doc-mode conflict.
##########
be/test/CMakeLists.txt:
##########
@@ -54,6 +54,12 @@ if (ENABLE_TLS)
list(APPEND UT_FILES ${TLS_UT_FILES})
endif()
+if (ENABLE_NESTED_GROUP)
+ file(GLOB_RECURSE NESTED_GROUP_UT_FILES CONFIGURE_DEPENDS
+ "${CMAKE_CURRENT_SOURCE_DIR}/${NESTED_GROUP_MODULE_DIR}/*.cpp")
+ list(APPEND UT_FILES ${NESTED_GROUP_UT_FILES})
+endif()
Review Comment:
The glob path is rooted at `be/test` (`CMAKE_CURRENT_SOURCE_DIR`), but
`NESTED_GROUP_MODULE_DIR` is defined as a directory under `be/src`. As written,
this will look for tests under `be/test/<module_dir>` instead of
`be/src/<module_dir>` and likely won’t pick up any module UTs. Adjust the path
to point into `../src/${NESTED_GROUP_MODULE_DIR}` (or otherwise correctly root
it at `be/src`).
--
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]
