github-actions[bot] commented on code in PR #67784:
URL: https://github.com/apache/doris/pull/67784#discussion_r4017645612
##########
be/src/core/column/column_varbinary.cpp:
##########
@@ -28,10 +28,103 @@
#include "core/column/column_string.h"
#include "core/column/columns_common.h"
#include "core/data_type/primitive_type.h"
+#include "exec/common/sip_hash.h"
#include "exec/sort/sort_block.h"
+#include "util/hash_util.hpp"
namespace doris {
#include "common/compile_check_begin.h"
+
+void ColumnVarbinary::insert_many_continuous_binary_data(const char* data,
const uint32_t* offsets,
+ size_t num) {
+ reserve(size() + num);
+ for (size_t row = 0; row < num; ++row) {
+ insert_data(data + offsets[row], offsets[row + 1] - offsets[row]);
+ }
+}
+
+void ColumnVarbinary::insert_many_dict_data(const int32_t* data_array, size_t
start_index,
+ const StringRef* dict, size_t
data_num,
+ uint32_t dict_num) {
+ reserve(size() + data_num);
+ // Decoder pages can be released after the call; copy long dictionary
entries into our arena.
+ for (size_t row = start_index; row < start_index + data_num; ++row) {
+ const auto& value = dict[data_array[row]];
+ insert_data(value.data, value.size);
+ }
+}
+
+// Hash the payload rather than StringView's representation: long values
contain pointers,
+// while short values are inline. Equal binary keys must partition identically
on every BE.
+void ColumnVarbinary::update_hash_with_value(size_t n, SipHash& hash) const {
+ const auto value = get_data_at(n);
+ hash.update(reinterpret_cast<const char*>(&value.size),
sizeof(value.size));
+ hash.update(value.data, value.size);
+}
+
+void ColumnVarbinary::update_hashes_with_value(uint64_t* __restrict hashes,
+ const uint8_t* __restrict
null_data) const {
+ for (size_t row = 0; row < size(); ++row) {
+ if (null_data == nullptr || null_data[row] == 0) {
+ const auto value = get_data_at(row);
+ hashes[row] = HashUtil::xxHash64WithSeed(value.data, value.size,
hashes[row]);
+ }
+ }
+}
+
+void ColumnVarbinary::update_xxHash_with_value(size_t start, size_t end,
uint64_t& hash,
+ const uint8_t* __restrict
null_data) const {
+ for (size_t row = start; row < end; ++row) {
+ if (null_data == nullptr || null_data[row] == 0) {
+ const auto value = get_data_at(row);
+ hash = HashUtil::xxHash64WithSeed(value.data, value.size, hash);
+ }
+ }
+}
+
+void ColumnVarbinary::update_crcs_with_value(uint32_t* __restrict hashes,
PrimitiveType type,
Review Comment:
[P1] Add VARBINARY to native tablet-routing CRC
This column CRC override is not used by ordinary OLAP tablet selection.
`VOlapTablePartitionParam::find_tablets` calls `RawValue::zlib_crc32(...,
TYPE_VARBINARY, ...)`, whose switch has no binary case; release builds return 0
(debug builds DCHECK), so a sole non-null VARBINARY distribution key always
routes to bucket zero. FE equality/IN pruning still computes a CRC, and high
bytes are separately re-encoded through ISO-8859-1 String -> UTF-8, so it can
select a different tablet and silently miss rows. Please hash raw bytes in both
`RawValue::zlib_crc32` and legacy `VarBinaryLiteral.getHashValue`, then cover
actual multi-bucket inserts and literal pruning for ASCII and high-byte values.
##########
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] Complete or fence the native VARBINARY delete path
This new native field type is accepted by `DeleteFromCommand`, but a simple
`DELETE ... WHERE vb = X'...'` is dispatched to `DeleteHandler`, whose value
validation has no `OLAP_FIELD_TYPE_VARBINARY` case and rejects every non-null
value. The later IN conversion and comparison/IN predicate factories omit
`TYPE_VARBINARY` as well. Also, `TCondition.condition_values` is
`list<string>`, so the current raw-byte -> ISO-8859-1 Java String -> Thrift
UTF-8 route cannot preserve high bytes. Please either route/fence these deletes
or add an explicit byte-safe representation through validation and predicate
creation, with equality/IN tests for empty, NUL, and high-byte values.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/VarBinaryLiteral.java:
##########
@@ -165,6 +146,7 @@ public boolean equals(Object o) {
@Override
protected int computeHashCode() {
- return Objects.hash(super.computeHashCode(), byteValues);
+ // equals compares content regardless of the byte array's identity or
declared length.
+ return Arrays.hashCode(byteValues);
Review Comment:
[P2] Make the fast child hash content-based too
This fixes the literal's regular `hashCode`, but it still inherits
`Literal.fastChildrenHashCode()`, which calls `Objects.hashCode(getValue())`.
Because `getValue()` is a `byte[]`, that is an identity hash. Parent
expressions cache this value, reject equality when it differs before comparing
children, and incorporate it into their own hash; two separately parsed but
identical binary predicates can therefore fail structural equality and Set/Map
lookup. Override `fastChildrenHashCode()` with the same content hash (as
`StringLikeLiteral` does) and test equal parent predicates built from separate
arrays.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/CreateTableInfo.java:
##########
@@ -470,9 +470,6 @@ public void validate(ConnectContext ctx) {
throw new AnalysisException(
"Disable to create table column with name start with
__DORIS_: " + columnNameUpperCase);
}
- if (columnDef.getType().isVarBinaryType()) {
- throw new AnalysisException("doris do not support varbinary
create table, could use it by catalog");
- }
if (columnDef.getType().isVariantType()) {
Review Comment:
[P2] Validate native VARBINARY defaults
Now that native VARBINARY columns are admitted, every present non-null
default still reaches `ColumnDef.validateDefaultValue`. That type switch has no
VARBINARY case and falls through to `Unsupported type: varbinary`, so a
definition such as `payload VARBINARY DEFAULT 'abc'` fails even though the BE
SerDe can materialize raw defaults. Please add binary-aware validation
(including declared byte-length checks) and a non-null default regression, or
explicitly fence defaults if they are intentionally unsupported.
--
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]
