github-actions[bot] commented on code in PR #65789:
URL: https://github.com/apache/doris/pull/65789#discussion_r3610681502
##########
be/src/common/config.cpp:
##########
@@ -75,6 +75,15 @@ DEFINE_Int32(arrow_flight_sql_port, "8050");
// Validate Arrow input buffers in opted-in Arrow readers before converting
them to Doris columns.
DEFINE_Bool(enable_arrow_input_validation, "true");
+// Max bytes of a single utf8/binary (int32-offset) column allowed in one
Arrow Flight result
+// RecordBatch. Arrow utf8/binary offsets are int32, so a column's data in one
batch must stay
+// below 2^31 (= INT32_MAX + 1). When a block would exceed this, the Arrow
Flight readers split
+// it by rows into multiple batches so each batch stays valid. Default is the
hard 2^31 limit;
+// only lower it (e.g. in tests) to exercise the split path without
materializing 2GB of data.
+DEFINE_mInt64(arrow_flight_result_max_utf8_bytes, "2147483648");
Review Comment:
[P1] Do not expose this validator through the broken mutable path
`DEFINE_mInt64` exposes this value through `/api/update_config`, but
`UPDATE_FIELD` invokes the registered zero-argument validator before assigning
`new_value`, so it validates the old default and then accepts `0`, `-1`, or a
value above 2^31. Zero makes every nonempty result hit the row-limit error,
while a negative value becomes a huge `size_t` and disables splitting; after
that, a valid recovery update is rejected because the invalid old value is what
gets validated. With `persist=true`, the unchecked value is also written to
`be_custom.conf` and can prevent the next startup. Since the comment says this
is lowered only in tests, please make it non-mutable, or fix the update path to
validate the candidate before committing it, and add `set_config` coverage.
##########
be/src/format/arrow/arrow_block_convertor.cpp:
##########
@@ -270,6 +277,109 @@ Status convert_to_arrow_batch(const Block& block, const
std::shared_ptr<arrow::S
return converter.convert(result);
}
+namespace {
+
+// Only utf8/binary use int32 offsets and can overflow at 2^31; fixed-size and
large_* cannot.
+bool is_int32_offset_binary_type(const std::shared_ptr<arrow::DataType>& type)
{
+ return type->id() == arrow::Type::STRING || type->id() ==
arrow::Type::BINARY;
+}
+
+// Byte length written into the arrow string/binary array for `row` (0 for
null), mirroring
+// DataTypeStringSerDeBase::write_column_to_arrow which appends
get_data_at(row).size bytes.
+size_t arrow_value_byte_size_at(const IColumn& column, size_t row) {
+ const IColumn* data = &column;
+ if (const auto* nullable = check_and_get_column<ColumnNullable>(column)) {
+ if (nullable->is_null_at(row)) {
+ return 0;
+ }
+ data = &nullable->get_nested_column();
+ }
+ return data->get_data_at(row).size;
+}
+
+} // namespace
+
+Status convert_to_arrow_batches(const Block& block, const
std::shared_ptr<arrow::Schema>& schema,
+ arrow::MemoryPool* pool,
+
std::vector<std::shared_ptr<arrow::RecordBatch>>* results,
+ const cctz::time_zone& timezone_obj) {
+ results->clear();
+ const int num_fields = schema->num_fields();
+ if (block.columns() != static_cast<size_t>(num_fields)) {
+ return Status::InvalidArgument("number fields not match");
+ }
+ const size_t num_rows = block.rows();
+ const auto max_bytes =
static_cast<size_t>(config::arrow_flight_result_max_utf8_bytes);
+
+ // Fast path: O(#string cols), each byte_size() is O(1) and >= the actual
arrow data size.
+ // If no int32-offset string/binary column reaches the limit, emit the
whole block as one
+ // batch -- zero added cost on the normal path, and this never misses an
overflow.
+ std::vector<int> big_fields;
+ for (int idx = 0; idx < num_fields; ++idx) {
+ if (is_int32_offset_binary_type(schema->field(idx)->type()) &&
+ block.get_by_position(idx).column->byte_size() >= max_bytes) {
+ big_fields.push_back(idx);
+ }
+ }
+ if (big_fields.empty() || num_rows == 0) {
+ std::shared_ptr<arrow::RecordBatch> batch;
+ RETURN_IF_ERROR(convert_to_arrow_batch(block, schema, pool, &batch,
timezone_obj));
+ results->push_back(std::move(batch));
+ return Status::OK();
+ }
+
+ // Materialize the (rarely) const columns once so per-row size probing is
cheap and correct.
+ std::vector<ColumnPtr> big_cols;
+ big_cols.reserve(big_fields.size());
+ for (int idx : big_fields) {
+
big_cols.push_back(block.get_by_position(idx).column->convert_to_full_column_if_const());
+ }
+
+ // Walk rows, cutting a batch right before any tracked column would reach
the int32 limit;
+ // the tightest-growing column decides each cut.
+ std::vector<size_t> running(big_cols.size());
+ std::vector<size_t> row_bytes(big_cols.size());
+ size_t start = 0;
+ while (start < num_rows) {
+ running.assign(running.size(), 0);
+ size_t end = start;
+ while (end < num_rows) {
+ bool fits = true;
+ for (size_t c = 0; c < big_cols.size(); ++c) {
+ const size_t rb = arrow_value_byte_size_at(*big_cols[c], end);
+ if (rb >= max_bytes) {
+ return Status::InternalError(
+ "Arrow Flight cannot send column '{}': a single
value at row {} is {} "
+ "bytes, exceeding the Arrow utf8/binary int32
offset limit of {} "
+ "bytes.",
+ schema->field(big_fields[c])->name(), end, rb,
max_bytes);
+ }
+ row_bytes[c] = rb;
+ if (running[c] + rb >= max_bytes) {
Review Comment:
[P1] Bound the complete Flight IPC body, not only value bytes
The exact per-buffer threshold is covered separately; even after that is
corrected, Arrow 17's `FlightPayload::Validate()` rejects an
`ipc_message.body_length` above `INT32_MAX`, while this cut accounts only for
one tracked value buffer. Offsets, validity buffers, padding, fixed-width
columns, and other individually sub-limit string columns all contribute to the
same body. The existing BigString test is already a counterexample: its
2,147,483,640 value bytes pass this `< 2^31` check by 8 bytes, but the offset
buffer makes the Flight payload too large even though `RecordBatch::Validate()`
succeeds. Please size/cut for the complete IPC body as well and add a
Flight-payload-level test.
--
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]
