Copilot commented on code in PR #64679:
URL: https://github.com/apache/doris/pull/64679#discussion_r3450634085
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -5306,8 +5306,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 silently disables doc mode and several sparse/subcolumn-related
properties whenever `enableNestedGroup` is true. That conflicts with the
updated parser test expecting an error when `variant_enable_nested_group` and
`variant_enable_doc_mode` are both set to true, and it can also surprise users
because their explicitly provided properties are ignored. Consider explicitly
detecting the conflict (when both are true) and throwing a
`NotSupportedException` with the expected message, rather than overriding the
user’s configuration.
##########
build.sh:
##########
@@ -701,6 +710,8 @@ if [[ "${BUILD_BE}" -eq 1 ]]; then
-DDORIS_JAVA_HOME="${JAVA_HOME}" \
-DBUILD_AZURE="${BUILD_AZURE}" \
-DWITH_TDE_DIR="${WITH_TDE_DIR}" \
+ -DENABLE_NESTED_GROUP="${ENABLE_NESTED_GROUP}" \
+ -DNESTED_GROUP_MODULE_DIR="${WITH_NESTED_GROUP_DIR}" \
Review Comment:
`NESTED_GROUP_MODULE_DIR` is described in CMake as 'directory under be/src',
but `WITH_NESTED_GROUP_DIR` (from the environment) can easily be set to an
absolute path or a path relative to the repo root. This mismatch can cause
confusing build failures or incorrect globbing paths. Consider either (a)
validating in `build.sh` that `WITH_NESTED_GROUP_DIR` is a path relative to
`be/src`, or (b) updating the CMake option description and CMakeLists path
joins to support absolute paths robustly (and checking that the directory
exists).
##########
be/test/CMakeLists.txt:
##########
@@ -23,6 +23,12 @@ set(EXECUTABLE_OUTPUT_PATH "${BUILD_DIR}/test")
file(GLOB_RECURSE UT_FILES CONFIGURE_DEPENDS *.cpp)
+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:
`NESTED_GROUP_MODULE_DIR` is documented in `be/CMakeLists.txt` as a
directory under `be/src`, but here the glob is rooted at `be/test`
(`CMAKE_CURRENT_SOURCE_DIR`). This will fail to pick up module sources unless
the module is also duplicated under `be/test`. Consider rooting the glob under
the `be/src` tree (e.g.,
`${CMAKE_CURRENT_SOURCE_DIR}/../src/${NESTED_GROUP_MODULE_DIR}`) or
clarifying/renaming the variable to indicate it is relative to `be/test`.
##########
fe/fe-common/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) {
+ sb.append(",");
+ sb.append("\"variant_enable_nested_group\" = \"")
+ .append(String.valueOf(enableNestedGroup)).append("\"");
+ }
Review Comment:
The unconditional `sb.append(\",\")` can introduce a leading comma when
`variant_enable_nested_group` is the first/only serialized property, producing
invalid `PROPERTIES(, ...)` SQL. Use the same delimiter strategy as the other
fields (e.g., track whether a property has already been appended, or append a
comma only if the last appended character isn’t `(`).
##########
be/src/common/config.cpp:
##########
@@ -1164,7 +1164,7 @@ DEFINE_mBool(enable_variant_doc_sparse_write_subcolumns,
"true");
// 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, "false");
+DEFINE_mBool(variant_nested_group_discard_scalar_on_conflict, "true");
Review Comment:
Changing the default of `variant_nested_group_discard_scalar_on_conflict`
from `false` to `true` is a behavior change that may impact existing workloads
(scalar values may now be dropped during conflicts by default). This should be
accompanied by a clear migration note (config docs/release notes) or gated
behind the NestedGroup build/feature flag if the new default is only intended
for NestedGroup-enabled deployments.
--
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]
