github-actions[bot] commented on code in PR #67784:
URL: https://github.com/apache/doris/pull/67784#discussion_r4021938317
##########
be/src/storage/olap_common.h:
##########
@@ -170,6 +170,8 @@ enum class FieldType {
OLAP_FIELD_TYPE_IPV4 = 38,
OLAP_FIELD_TYPE_IPV6 = 39,
OLAP_FIELD_TYPE_TIMESTAMPTZ = 40,
+ // A distinct persisted type keeps binary payloads out of character
conversion paths.
+ OLAP_FIELD_TYPE_VARBINARY = 41,
Review Comment:
[P1] Reserve a cross-branch-safe persisted storage type ID
`ColumnMetaPB.type` persists this numeric `FieldType`, and segment readers
cast it back before choosing the data type and decoder. Current master already
assigns value 41 to `OLAP_FIELD_TYPE_TIMESTAMP_NS`, so a tablet written here
with native VARBINARY will be read as a different physical type after an
upgrade to master (or vice versa on rollback), producing a type mismatch or
selecting the wrong decoder. Please allocate one globally unique persisted ID
across the maintained branches before this version writes VARBINARY segments,
and cover the forward/rollback metadata boundary in a compatibility test.
##########
fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java:
##########
@@ -207,6 +207,8 @@ public abstract class Type {
arraySubTypes.add(CHAR);
arraySubTypes.add(VARCHAR);
arraySubTypes.add(STRING);
+ // Nested binary leaves retain the same byte-preserving storage as
scalar VARBINARY.
+ arraySubTypes.add(VARBINARY);
Review Comment:
[P1] Complete VARBINARY dispatch for collection functions
This makes `ARRAY<VARBINARY>` a supported native type, but the newly valid
collection operations still lack BE dispatch.
`array_contains`/`array_position`/`array_distinct` select neither their
`is_string_type` path nor a `dispatch_switch_scalar` case, so they return
`unsupported types`. Independently, Nereids accepts `collect_set(payload)` with
an `ARRAY<VARBINARY>` result, but `dispatch_switch_all` also omits
`TYPE_VARBINARY` and the distinct aggregate fallback throws `INTERNAL_ERROR:
unexpected type for collect`. Please add byte-owning `ColumnVarbinary` handling
to both dispatch families (and audit their siblings), or reject these functions
during analysis, with empty/NUL/high-byte regressions.
##########
be/src/exec/sink/vtablet_block_convertor.cpp:
##########
@@ -330,6 +332,26 @@ Status
OlapTableBlockConvertor::_internal_validate_column(RuntimeState* state, B
};
switch (type->get_primitive_type()) {
+ case TYPE_VARBINARY: {
+ const auto* binary_type =
+ assert_cast<const
DataTypeVarbinary*>(remove_nullable(type).get());
+ const auto* binary = assert_cast<const
ColumnVarbinary*>(real_column_ptr.get());
+ int limit = config::string_type_length_soft_limit_bytes;
+ if (binary_type->len() >= 0) {
+ limit = std::min(limit, binary_type->len());
+ }
+ // VARBINARY(n) limits bytes, including nested leaves; text truncation
would corrupt keys.
+ for (size_t j = 0; j < row_count; ++j) {
+ const auto row = rows ? (*rows)[j] : j;
+ if (need_to_validate(j, row, _filter_map, null_map) &&
Review Comment:
[P1] Preserve the parent-row mapping through nested STRUCT validation
ARRAY/MAP validation builds a permutation from each flattened child back to
the top-level input row, but the STRUCT branch drops that `rows` mapping before
recursing. This new bounded VARBINARY loop then indexes the top-level
`_filter_map` with the flattened child ordinal. For example, one row containing
three `STRUCT<b:VARBINARY(2)>` array elements reads `_filter_map[1]` and `[2]`
out of bounds even when the values are valid; an oversized value can also
filter the wrong row. Please forward the current mapping through STRUCT child
validation (including its string helper) and add a deep nested strict-load
regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/CreateTableInfo.java:
##########
@@ -561,7 +558,8 @@ public void validate(ConnectContext ctx) {
break;
}
keys.add(column.getName());
- if (type.isVarcharType()) {
+ // Variable-length binary keys, like VARCHAR,
terminate the short-key prefix.
+ if (type.isVarcharType() ||
type.isVarBinaryType()) {
Review Comment:
[P1] Keep VARBINARY terminal in every short-key selection path
This inference path recognizes that VARBINARY must end a variable-width key
prefix, but `Env.calcShortKeyColumnCount` still treats only VARCHAR that way
for explicit keys/cluster keys and user-specified `short_key`. Thus `DUPLICATE
KEY(vb,id)` can select both columns. The BE coder concatenates raw, unescaped
VARBINARY bytes with the next column marker; `X'01'` followed by another column
has `0x02` where `X'0100'` has `0x00`, reversing the declared tuple order.
`ShortKeyIndex` then binary-searches those sampled bytes as if they were
sorted, so equality/range seeks can skip rows. Please treat VARBINARY like
VARCHAR in all automatic/manual/cluster selection paths and add prefix-value
composite-key regressions.
##########
be/src/storage/key_coder.h:
##########
@@ -356,6 +356,10 @@ class KeyCoderTraits<FieldType::OLAP_FIELD_TYPE_STRING> {
}
};
+template <>
+class KeyCoderTraits<FieldType::OLAP_FIELD_TYPE_VARBINARY>
Review Comment:
[P1] Make full VARBINARY keys memcomparable
This inherits VARCHAR's raw `full_encode_ascending`, but MOW's primary index
concatenates every full key column and assumes those bytes stay sorted. For
`UNIQUE KEY(vb,id)`, logical `(X'01', id)` sorts before `(X'0100', id)`, while
the shorter encoded value reaches the next column's `0x02` marker where the
longer value still has `0x00`, reversing their encoded order. The writer checks
this only with `DCHECK`, and indexed-column seeks later binary-search the
resulting keys, so upserts/deletes can miss an existing key. Making VARBINARY
terminal only in the short-key count does not fix this full-key index. Please
use a prefix-free order-preserving full encoding (and matching seek encoding),
or reject layouts with a suffix after VARBINARY, with composite MOW
prefix-value regressions.
--
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]
