Copilot commented on code in PR #61047:
URL: https://github.com/apache/doris/pull/61047#discussion_r2883802100
##########
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));
Review Comment:
`Integer.parseInt(...)` here can throw `NumberFormatException` and escape as
a runtime exception, which bypasses the intended `AnalysisException` flow for
invalid properties. Wrap parsing in a try/catch and rethrow as
`AnalysisException` (similar to other property analyzers in this file).
```suggestion
String maxSubcolumnsCountStr =
properties.get(PROPERTIES_VARIANT_MAX_SUBCOLUMNS_COUNT);
int count;
try {
count = Integer.parseInt(maxSubcolumnsCountStr);
} catch (NumberFormatException e) {
throw new AnalysisException("Invalid value for
variant_max_subcolumns_count: "
+ maxSubcolumnsCountStr + ", it must be a
non-negative integer", e);
}
```
##########
fe/fe-common/src/main/java/org/apache/doris/catalog/VariantType.java:
##########
@@ -218,7 +227,8 @@ public boolean equals(Object other) {
&& variantMaxSubcolumnsCount ==
otherVariantType.variantMaxSubcolumnsCount
&& enableTypedPathsToSparse ==
otherVariantType.enableTypedPathsToSparse
&& enableVariantDocMode ==
otherVariantType.enableVariantDocMode
- && variantDocMaterializationMinRows ==
otherVariantType.variantDocMaterializationMinRows;
+ && variantDocMaterializationMinRows ==
otherVariantType.variantDocMaterializationMinRows
+ && enableNestedGroup == otherVariantType.enableNestedGroup;
}
@Override
Review Comment:
`VariantType` overrides `equals` but does not override `hashCode`, so
instances that compare equal can still have different hash codes (it will use
`ScalarType.hashCode()` which ignores VARIANT properties). This can break
behavior in hash-based collections; add a `hashCode()` implementation
consistent with `equals` (including `enableNestedGroup` and any other fields
that should define equality).
```suggestion
@Override
public int hashCode() {
return Objects.hash(predefinedFields,
variantMaxSubcolumnsCount,
enableTypedPathsToSparse,
enableVariantDocMode,
variantDocMaterializationMinRows,
enableNestedGroup);
}
@Override
```
##########
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:
New nested-group VARIANT properties parsing/validation is introduced here,
but there are existing unit tests for `PropertyAnalyzer` (e.g.
`fe-core/src/test/java/.../PropertyAnalyzerTest.java`) and none appear to cover
`variant_enable_nested_group`. Add tests for: valid true/false parsing, invalid
value rejection, and the mutual-exclusion rules enforced in
`validateVariantProperties`.
##########
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;
Review Comment:
`defaultVariantEnableNestedGroup` is initialized to `true`, but nested group
is explicitly rejected in the parser (`NotSupportedException`). This default
will effectively break VARIANT parsing/DDL unless the user turns it off. Set
the default to `false` until the feature is actually supported end-to-end (and
align with proto default `false`).
```suggestion
public boolean defaultVariantEnableNestedGroup = false;
```
##########
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();
Review Comment:
`enableNestedGroup` is defaulted to `true` when `ConnectContext` is null,
and the session default is also currently `true`. Since the code
unconditionally throws `NotSupportedException` when `enableNestedGroup` is
true, this makes VARIANT parsing fail by default. Default this flag to `false`
(until supported) and/or only throw when the user explicitly sets
`variant_enable_nested_group=true` in the properties.
```suggestion
// Nested group for VARIANT is not supported by default; only enable
when explicitly requested
boolean enableNestedGroup = 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:
`Boolean.parseBoolean(...)` never throws, so invalid values like "yes" will
be silently parsed as `false` and won’t raise `AnalysisException` as the error
message suggests. Implement strict parsing (accept only case-insensitive
"true"/"false"; otherwise throw) so users get correct validation for
`variant_enable_nested_group`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -5173,10 +5176,17 @@ public DataType
visitVariantPredefinedFields(VariantPredefinedFieldsContext ctx)
.analyzeVariantDocMaterializationMinRows(properties,
variantDocMaterializationMinRows);
variantDocHashShardCount = PropertyAnalyzer
.analyzeVariantDocHashShardCount(properties,
variantDocHashShardCount);
+ enableNestedGroup = PropertyAnalyzer
+ .analyzeEnableNestedGroup(properties, enableNestedGroup);
} catch (org.apache.doris.common.AnalysisException e) {
throw new NotSupportedException(e.getMessage());
}
+ if (enableNestedGroup) {
+ throw new NotSupportedException(
+ "variant_enable_nested_group is not supported now");
+ }
Review Comment:
This `NotSupportedException` currently triggers whenever `enableNestedGroup`
is true, which (given the current defaults) can block creation/parsing of
VARIANT columns even when users did not request nested group support. Consider
gating this error on the presence of the property in the input (or defaulting
the session/ConnectContext fallback to false) so existing behavior isn’t broken.
--
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]
