github-actions[bot] commented on code in PR #63528:
URL: https://github.com/apache/doris/pull/63528#discussion_r4226698280
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ColumnDefinition.java:
##########
@@ -630,16 +629,68 @@ private void validateInternal(boolean isOlap, Set<String>
keysSet, Set<String> c
* Validate non-null defaults for complex types before connector-specific
validation.
*/
public void validateComplexTypeDefaultValue() throws AnalysisException {
- if (!defaultValue.isPresent() || defaultValue.get() ==
DefaultValue.NULL_DEFAULT_VALUE) {
- return;
- }
- if (type.isMapType()) {
- throw new AnalysisException("Map type column default value just
support null");
+ if (type.isArrayType()) {
+ validateArrayDefaultValue();
+ } else if (type.isMapType()) {
+ validateMapDefaultValue();
Review Comment:
[P1] Escape quoted complex defaults in rendered table DDL. This validation
now accepts values such as `MAP<STRING, INT> DEFAULT '{"a": 10}'`, but
`Column.toSql()` wraps the raw persisted default in double quotes without
escaping its interior quotes. `SHOW CREATE TABLE` consequently emits `DEFAULT
"{"a": 10}"`, and `CREATE TABLE LIKE` fails when it reparses that DDL. Please
render the value with SQL-literal escaping and add a SHOW CREATE/LIKE
round-trip case.
##########
be/src/core/data_type_serde/data_type_map_serde.cpp:
##########
@@ -756,6 +756,68 @@ Status DataTypeMapSerDe::_from_string(StringRef& str,
IColumn& column,
return Status::OK();
}
+Status DataTypeMapSerDe::from_fe_string(const std::string& str, Field& field)
const {
+ StringRef slice(str);
+ slice = slice.trim_whitespace();
+ if (slice.empty()) {
+ return Status::InvalidArgument("slice is empty!");
+ }
+ if (slice.front() != '{') {
+ std::stringstream ss;
+ ss << slice.front() << '\'';
+ return Status::InvalidArgument("Map does not start with '{' character,
found '" + ss.str());
+ }
+ if (slice.back() != '}') {
+ std::stringstream ss;
+ ss << slice.back() << '\'';
+ return Status::InvalidArgument("Map does not end with '}' character,
found '" + ss.str());
+ }
+
+ Array keys;
+ Array values;
+ if (slice.size > 2) {
+ slice = slice.substring(1, slice.size - 2);
+ slice = slice.trim_whitespace();
+ if (!slice.empty()) {
+ FormatOptions options;
+ auto split_result =
ComplexTypeDeserializeUtil::split_by_delimiter(slice, [&](char c) {
+ return c == options.map_key_delim || c ==
options.collection_delim;
+ });
+ if (split_result.size() % 2 != 0) {
+ return Status::InvalidArgument("Map does not have even number
of key-value pairs");
+ }
+ keys.reserve(split_result.size() / 2);
+ values.reserve(split_result.size() / 2);
+ for (int i = 0; i < split_result.size(); i += 2) {
+ if (split_result[i].delimiter != options.map_key_delim) {
+ return Status::InvalidArgument(
+ "Map key-value pair does not have map key
delimiter");
+ }
+ if (i != 0 && split_result[i - 1].delimiter !=
options.collection_delim) {
+ return Status::InvalidArgument(
+ "Map key-value pair does not have collection
delimiter");
+ }
+
+ Field key_field;
+ Field value_field;
+ RETURN_IF_ERROR(ComplexTypeDeserializeUtil::process_field(
+ key_serde, split_result[i].element, key_field));
+ RETURN_IF_ERROR(ComplexTypeDeserializeUtil::process_field(
+ value_serde, split_result[i + 1].element,
value_field));
+ keys.push_back(std::move(key_field));
Review Comment:
[P2] Match MAP default semantics for repeated keys. `MAP<STRING,INT> DEFAULT
'{"a":1,"a":2}'` passes the new FE literal check, whose `visitMapLiteral()`
overwrites the first key and represents `{"a":2}`. This BE loop instead appends
both pairs, so old rows or schema-change backfills return `{"a":1, "a":2}`.
Please either normalize repeated keys like the FE literal or reject them
consistently, and cover this case through default materialization.
##########
be/src/core/data_type_serde/complex_type_deserialize_util.h:
##########
@@ -80,13 +81,37 @@ struct ComplexTypeDeserializeUtil {
static bool is_null_string(const StringRef& str) {
if (str.size == 4) {
- // null
- return str.data[0] == 'n' && str.data[1] == 'u' && str.data[2] ==
'l' &&
- str.data[3] == 'l';
+ // SQL NULL literal is case-insensitive.
+ return (str.data[0] == 'n' || str.data[0] == 'N') &&
+ (str.data[1] == 'u' || str.data[1] == 'U') &&
+ (str.data[2] == 'l' || str.data[2] == 'L') &&
+ (str.data[3] == 'l' || str.data[3] == 'L');
}
return false;
}
+ static Status process_field(const DataTypeSerDeSPtr& serde, StringRef str,
Field& field) {
+ str = str.trim_whitespace();
+ auto nullable_serde =
std::dynamic_pointer_cast<DataTypeNullableSerDe>(serde);
+ if (is_null_string(str)) {
+ if (nullable_serde == nullptr) {
+ return Status::InvalidArgument(
+ "NULL default is not allowed for non-nullable complex
field");
+ }
+ field = Field::create_field<TYPE_NULL>(Null {});
+ return Status::OK();
+ }
+ auto str_without_quote = str.trim_quote();
Review Comment:
[P1] Decode quoted nested string values before materializing defaults. FE
accepts a value such as `ARRAY<STRING> DEFAULT '["a""b"]'` as an array literal
and interprets its element as `a"b`, but `trim_quote()` only removes the outer
quotes. The string serde then stores `a""b` in rows read through the default
iterator (and schema-change backfill). Backslash escapes have the same
mismatch. Please apply SQL string-literal decoding and cover an escaped value
on the old-rowset read path.
##########
be/src/core/data_type_serde/complex_type_deserialize_util.h:
##########
@@ -80,13 +81,37 @@ struct ComplexTypeDeserializeUtil {
static bool is_null_string(const StringRef& str) {
if (str.size == 4) {
- // null
- return str.data[0] == 'n' && str.data[1] == 'u' && str.data[2] ==
'l' &&
- str.data[3] == 'l';
+ // SQL NULL literal is case-insensitive.
+ return (str.data[0] == 'n' || str.data[0] == 'N') &&
+ (str.data[1] == 'u' || str.data[1] == 'U') &&
+ (str.data[2] == 'l' || str.data[2] == 'L') &&
+ (str.data[3] == 'l' || str.data[3] == 'L');
}
return false;
}
+ static Status process_field(const DataTypeSerDeSPtr& serde, StringRef str,
Field& field) {
+ str = str.trim_whitespace();
+ auto nullable_serde =
std::dynamic_pointer_cast<DataTypeNullableSerDe>(serde);
+ if (is_null_string(str)) {
+ if (nullable_serde == nullptr) {
+ return Status::InvalidArgument(
+ "NULL default is not allowed for non-nullable complex
field");
+ }
+ field = Field::create_field<TYPE_NULL>(Null {});
+ return Status::OK();
+ }
+ auto str_without_quote = str.trim_quote();
+ auto nested_serde = serde;
+ if (nullable_serde != nullptr) {
+ // Complex default values need strict nested conversion.
DataTypeNullableSerDe
+ // converts nested parse failures to NULL, which would silently
accept
+ // type-mismatched default literals.
+ nested_serde = nullable_serde->get_nested_serde();
+ }
+ return nested_serde->from_fe_string(str_without_quote.to_string(),
field);
Review Comment:
[P1] Interpret nested literals before scalar default conversion.
`ARRAY<DATEV2> DEFAULT '[DATEV2 "2024-01-01"]'` passes FE's outer array check,
but this forwards the raw typed token to the DATEV2 serde; its parse fails and
returns OK with `MIN_DATE_V2`, silently corrupting old-row reads and
schema-change backfills. `ARRAY<INT> DEFAULT '[1e3]'` is likewise accepted by
FE as numeric 1000, yet the INT serde rejects the raw exponent token and those
reads fail. `BINARY "abc"` retains its prefix too. Please materialize FE
literal values rather than forwarding SQL token spelling, and cover these cases
through old-rowset reads.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ColumnDefinition.java:
##########
@@ -630,16 +629,68 @@ private void validateInternal(boolean isOlap, Set<String>
keysSet, Set<String> c
* Validate non-null defaults for complex types before connector-specific
validation.
*/
public void validateComplexTypeDefaultValue() throws AnalysisException {
- if (!defaultValue.isPresent() || defaultValue.get() ==
DefaultValue.NULL_DEFAULT_VALUE) {
- return;
- }
- if (type.isMapType()) {
- throw new AnalysisException("Map type column default value just
support null");
+ if (type.isArrayType()) {
+ validateArrayDefaultValue();
+ } else if (type.isMapType()) {
+ validateMapDefaultValue();
} else if (type.isStructType()) {
- throw new AnalysisException("Struct type column default value just
support null");
+ validateStructDefaultValue();
} else if (type.isJsonType() || type.isVariantType()) {
- throw new AnalysisException("Json or Variant type column default
value just support null");
+ if (hasNonNullDefaultValue()) {
+ throw new AnalysisException("Json or Variant type column
default value only supports DEFAULT NULL");
+ }
+ }
+ }
+
+ private void validateArrayDefaultValue() {
+ if (!hasNonNullDefaultValue()) {
+ return;
}
+ if (!isLiteralDefaultValue(ArrayLiteral.class)) {
+ throw new AnalysisException("Array type column default value only
supports array literals or DEFAULT NULL");
+ }
+ }
+
+ private void validateMapDefaultValue() {
+ if (!hasNonNullDefaultValue()) {
+ return;
+ }
+ if (!isLiteralDefaultValue(MapLiteral.class)) {
Review Comment:
[P2] Handle new complex defaults in load `replace_value`. A MAP column can
now have a nonnull default such as `{"a":1}`, but a one-argument
`replace_value(NULL)` mapping takes that value and calls
`ColumnDef.validateDefaultValue()`, whose scalar-only precondition throws
`IllegalArgumentException`. The load fails before the provider can parse the
default into its replacement expression. Please validate this fallback with the
complex-default path (or reject it with a deliberate user error) and cover the
mapping in a regression.
--
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]
