github-actions[bot] commented on code in PR #61047:
URL: https://github.com/apache/doris/pull/61047#discussion_r2883808182
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -3443,6 +3445,13 @@ public boolean isEnableESParallelScroll() {
)
public int defaultVariantDocHashShardCount = 64;
+ @VariableMgr.VarAttr(
+ name = DEFAULT_VARIANT_ENABLE_NESTED_GROUP,
+ needForward = true,
+ fuzzy = true
+ )
+ public boolean defaultVariantEnableNestedGroup = true;
+
@VariableMgr.VarAttr(
name = "use_v3_storage_format",
fuzzy = true,
Review Comment:
**[Bug]** The default value `true` is problematic. Since
`LogicalPlanBuilder.visitVariantPredefinedFields()` throws
`NotSupportedException("variant_enable_nested_group is not supported now")`
when this is `true`, any user creating a `variant<...>` column (with predefined
fields or properties) will fail by default — they would need to explicitly set
`SET default_variant_enable_nested_group = false` first.
Since the feature is explicitly gated as unsupported, the default should be
`false`:
```java
public boolean defaultVariantEnableNestedGroup = false;
```
The fallback value in `LogicalPlanBuilder.java` line 5158
(`ConnectContext.get() == null ? true`) should also be `false` for consistency.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -5155,6 +5155,9 @@ public DataType
visitVariantPredefinedFields(VariantPredefinedFieldsContext ctx)
int variantDocHashShardCount = ConnectContext.get() == null ? 128 :
ConnectContext.get().getSessionVariable().getDefaultVariantDocHashShardCount();
+ boolean enableNestedGroup = ConnectContext.get() == null ? true :
+
ConnectContext.get().getSessionVariable().getDefaultVariantEnableNestedGroup();
+
try {
// validate properties: variant_enable_doc_mode cannot be set
together with other properties
Review Comment:
**[Bug]** The fallback default when `ConnectContext.get()` is `null` is
`true`, which is inconsistent — it would immediately trigger the "not supported
now" exception on line 5185-5188. This should be `false` to match a safe
default:
```java
boolean enableNestedGroup = ConnectContext.get() == null ? false :
```
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java:
##########
@@ -2063,6 +2064,21 @@ public static boolean
analyzeEnableTypedPathsToSparse(Map<String, String> proper
return enableTypedPathsToSparse;
}
+ public static boolean analyzeEnableNestedGroup(Map<String, String>
properties,
+ boolean defaultValue) throws AnalysisException {
+ boolean enableNestedGroup = defaultValue;
+ if (properties != null &&
properties.containsKey(PROPERTIES_VARIANT_ENABLE_NESTED_GROUP)) {
+ String enableNestedGroupStr =
properties.get(PROPERTIES_VARIANT_ENABLE_NESTED_GROUP);
+ try {
+ enableNestedGroup = Boolean.parseBoolean(enableNestedGroupStr);
+ } catch (Exception e) {
+ throw new AnalysisException("variant_enable_nested_group must
be `true` or `false`");
+ }
+ properties.remove(PROPERTIES_VARIANT_ENABLE_NESTED_GROUP);
+ }
Review Comment:
**[Dead Code]** `Boolean.parseBoolean()` in Java **never throws an
exception** — it returns `false` for any string that is not case-insensitive
`"true"`. This `catch` block is unreachable dead code. The validation should be
explicit instead:
```java
if (!"true".equalsIgnoreCase(enableNestedGroupStr)
&& !"false".equalsIgnoreCase(enableNestedGroupStr)) {
throw new AnalysisException("variant_enable_nested_group must be `true`
or `false`");
}
enableNestedGroup = Boolean.parseBoolean(enableNestedGroupStr);
```
Note: This is a pre-existing pattern (same issue in
`analyzeEnableTypedPathsToSparse` and `analyzeEnableVariantDocMode`), but since
this is new code, it should be done correctly.
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java:
##########
@@ -2176,6 +2192,23 @@ public static void validateVariantProperties(Map<String,
String> properties) thr
+ "cannot be set together");
}
}
+ // variant_enable_nested_group=true is mutually exclusive with
+ // variant_enable_doc_mode=true and variant_max_subcolumns_count>0
+ if (properties != null &&
properties.containsKey(PROPERTIES_VARIANT_ENABLE_NESTED_GROUP)
+ &&
"true".equalsIgnoreCase(properties.get(PROPERTIES_VARIANT_ENABLE_NESTED_GROUP)))
{
+ if (properties.containsKey(PROPERTIES_VARIANT_ENABLE_DOC_MODE)
+ &&
"true".equalsIgnoreCase(properties.get(PROPERTIES_VARIANT_ENABLE_DOC_MODE))) {
+ throw new AnalysisException("variant_enable_nested_group and
variant_enable_doc_mode "
+ + "cannot both be true");
+ }
+ if
(properties.containsKey(PROPERTIES_VARIANT_MAX_SUBCOLUMNS_COUNT)) {
+ int count =
Integer.parseInt(properties.get(PROPERTIES_VARIANT_MAX_SUBCOLUMNS_COUNT));
+ if (count > 0) {
+ throw new AnalysisException("variant_enable_nested_group
cannot be true when "
+ + "variant_max_subcolumns_count > 0");
+ }
Review Comment:
**[Bug]** `Integer.parseInt()` can throw unchecked `NumberFormatException`
if `variant_max_subcolumns_count` contains a non-integer value. Since
`validateVariantProperties()` is called **before**
`analyzeVariantMaxSubcolumnsCount()` in `LogicalPlanBuilder.java` (line 5163 vs
5165), this `parseInt` runs before the proper validation. This would result in
an unhelpful `NumberFormatException` instead of a proper `AnalysisException`.
Wrap in a try-catch or use the already-parsed value:
```java
try {
int count =
Integer.parseInt(properties.get(PROPERTIES_VARIANT_MAX_SUBCOLUMNS_COUNT));
...
} catch (NumberFormatException e) {
// Let analyzeVariantMaxSubcolumnsCount handle the validation later
// or throw AnalysisException here
}
```
--
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]
