lxy-9602 commented on code in PR #275:
URL: https://github.com/apache/paimon-cpp/pull/275#discussion_r3922670955
##########
src/paimon/core/io/key_value_in_memory_record_reader.cpp:
##########
@@ -111,22 +111,42 @@ void KeyValueInMemoryRecordReader::Close() {
Result<std::shared_ptr<arrow::NumericArray<arrow::UInt64Type>>>
KeyValueInMemoryRecordReader::SortBatch() const {
std::vector<arrow::compute::SortKey> sort_keys;
+ arrow::FieldVector sort_fields;
+ arrow::ArrayVector sort_columns;
sort_keys.reserve(primary_keys_.size() +
user_defined_sequence_fields_.size());
+ sort_fields.reserve(primary_keys_.size() +
user_defined_sequence_fields_.size());
+ sort_columns.reserve(primary_keys_.size() +
user_defined_sequence_fields_.size());
+ const arrow::StructType* value_type = value_struct_array_->struct_type();
+ auto append_sort_key = [&](const std::string& name,
arrow::compute::SortOrder order) -> Status {
+ int32_t field_index = value_type->GetFieldIndex(name);
+ if (field_index < 0) {
+ return Status::Invalid(fmt::format("cannot find field {} in data
batch", name));
+ }
+ sort_keys.emplace_back(name, order);
+ sort_fields.push_back(value_type->field(field_index));
+ sort_columns.push_back(value_struct_array_->field(field_index));
+ return Status::OK();
+ };
for (const auto& name : primary_keys_) {
- sort_keys.emplace_back(name, arrow::compute::SortOrder::Ascending);
+ PAIMON_RETURN_NOT_OK(append_sort_key(name,
arrow::compute::SortOrder::Ascending));
}
const auto sequence_sort_order = sequence_fields_ascending_
? arrow::compute::SortOrder::Ascending
:
arrow::compute::SortOrder::Descending;
for (const auto& name : user_defined_sequence_fields_) {
- sort_keys.emplace_back(name, sequence_sort_order);
+ PAIMON_RETURN_NOT_OK(append_sort_key(name, sequence_sort_order));
}
auto sort_options =
arrow::compute::SortOptions(sort_keys,
arrow::compute::NullPlacement::AtStart);
arrow::compute::ExecContext exec_context(arrow_pool_.get());
- PAIMON_ASSIGN_OR_RAISE_FROM_ARROW(std::shared_ptr<arrow::Array>
sorted_indices,
-
arrow::compute::SortIndices(arrow::Datum(value_struct_array_),
-
sort_options, &exec_context));
+ // Arrow's StructArray sorting path may inspect value columns outside the
sort keys. Restrict
+ // the batch to the requested fields so non-sortable values, such as
VECTOR, are never compared.
+ std::shared_ptr<arrow::RecordBatch> sort_batch =
Review Comment:
I’d like to better understand what issues `inspect value columns` can cause
here. Does it lead to incorrect sorting, crashes, or explicit errors? Also, is
this considered a bug in Arrow, and is there any ongoing fix for it?
##########
test/inte/write_and_read_inte_test.cpp:
##########
@@ -69,6 +70,180 @@
#include "rapidjson/writer.h"
namespace paimon::test {
+namespace {
+
+struct PrimaryKeyVectorRow {
+ int64_t primary_key;
+ std::optional<std::array<float, 3>> embedding;
Review Comment:
Please try to avoid using this kind of struct unless the entire test uses a
fixed schema throughout. Otherwise, please use simple Arrow JSON to initialize
the input and expected Arrow arrays, as it is more straightforward and easier
to read.
--
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]
