This is an automated email from the ASF dual-hosted git repository.
pitrou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new bf108d4e68 GH-47252: [C++][Compute] Fix sort_indices for temporal
types in arrow::Table (#50270)
bf108d4e68 is described below
commit bf108d4e68d56d7a2b8b5421ce2602df2fb3e639
Author: nfrmtk <[email protected]>
AuthorDate: Mon Jun 29 18:17:58 2026 +0300
GH-47252: [C++][Compute] Fix sort_indices for temporal types in
arrow::Table (#50270)
### Rationale for this change
I was unable to use compute::SortIndices with timestamp type because of
crash.
### What changes are included in this PR?
Fix. The issue was that comparator for merging record batches was not
converting timestamp type to its physical variant.
so it crashed on null pointer reference on [checked_cast
result](https://github.com/apache/arrow/blob/d0919579cbdd37549ba163c0d61843f04bac92cd/cpp/src/arrow/compute/kernels/chunked_internal.h#L55)
### Are these changes tested?
yes
### Are there any user-facing changes?
**This PR contains a "Critical Fix".** (If the changes fix either (a) a
security vulnerability, (b) a bug that caused incorrect or invalid data to be
produced, or (c) a bug that causes a crash (even when the API contract is
upheld), please provide explanation. If not, you can remove this.)
i've already provided, i suppose
* GitHub Issue: #47252
Lead-authored-by: Ivan Lykov <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Co-authored-by: nfrmtk <[email protected]>
Co-authored-by: Copilot Autofix powered by AI
<[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
---
.../arrow/compute/kernels/vector_sort_internal.h | 5 +++--
cpp/src/arrow/compute/kernels/vector_sort_test.cc | 23 ++++++++++++++++++++++
2 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/cpp/src/arrow/compute/kernels/vector_sort_internal.h
b/cpp/src/arrow/compute/kernels/vector_sort_internal.h
index 06d911b2c0..9d2ad650f1 100644
--- a/cpp/src/arrow/compute/kernels/vector_sort_internal.h
+++ b/cpp/src/arrow/compute/kernels/vector_sort_internal.h
@@ -780,14 +780,15 @@ struct ResolvedTableSortKey {
// so we can't simply access the column from the table directly.
ArrayVector chunks;
chunks.reserve(batches.size());
+ auto physical_type = GetPhysicalType(f.type->GetSharedPtr());
int64_t null_count = 0;
for (const auto& batch : batches) {
ARROW_ASSIGN_OR_RAISE(auto child, f.path.GetFlattened(*batch));
null_count += child->null_count();
- chunks.push_back(std::move(child));
+ chunks.push_back(GetPhysicalArray(*child, physical_type));
}
- return ResolvedTableSortKey(f.type->GetSharedPtr(), std::move(chunks),
f.order,
+ return ResolvedTableSortKey(physical_type, std::move(chunks), f.order,
f.null_placement, null_count);
};
diff --git a/cpp/src/arrow/compute/kernels/vector_sort_test.cc
b/cpp/src/arrow/compute/kernels/vector_sort_test.cc
index 40bd093620..cd31c85062 100644
--- a/cpp/src/arrow/compute/kernels/vector_sort_test.cc
+++ b/cpp/src/arrow/compute/kernels/vector_sort_test.cc
@@ -1739,6 +1739,29 @@ TEST_F(TestTableSortIndices, Decimal) {
AssertSortIndices(table, options, "[3, 4, 0, 2, 1]");
}
+TEST_F(TestTableSortIndices, Timestamp) {
+ auto schema = ::arrow::schema({
+ {field("a", timestamp(TimeUnit::MICRO))},
+ {field("b", timestamp(TimeUnit::MICRO))},
+ });
+ std::vector<SortKey> sort_keys{SortKey("a", SortOrder::Ascending),
+ SortKey("b", SortOrder::Descending)};
+
+ auto table = TableFromJSON(schema, {R"([{"a": 1, "b": 2},
+ {"a": 3, "b": 4},
+ {"a": 8, "b": 6}
+ ])",
+ R"([{"a": 6, "b": null},
+ {"a": 6, "b": 7}
+ ])"});
+ SortOptions options(sort_keys);
+ AssertSortIndices(table, options, "[0, 1, 4, 3, 2]");
+ options.sort_keys[0].null_placement = NullPlacement::AtStart;
+ AssertSortIndices(table, options, "[0, 1, 4, 3, 2]");
+ options.sort_keys[1].null_placement = NullPlacement::AtStart;
+ AssertSortIndices(table, options, "[0, 1, 3, 4, 2]");
+}
+
TEST_F(TestTableSortIndices, NullType) {
auto schema = arrow::schema({
field("a", null()),
