Skip to content

Commit 43eca86

Browse files
authored
GH-46454: [C++][Dataset][Acero] Preserve order when writting with TeeNode (#46455)
### Rationale for this change TeeNode needs to sequence batches when implicit order within processed dataset. ### What changes are included in this PR? Conditionally sequence batches when preserve_order=true ### Are these changes tested? I tested it in my use case. No CI tests AFAIK. ### Are there any user-facing changes? Dataset will now be ordered as expected. * GitHub Issue: #46454 Authored-by: Rafał Hibner <rafal.hibner@secom.com.pl> Signed-off-by: Rossi Sun <zanmato1984@gmail.com>
1 parent f52076c commit 43eca86

2 files changed

Lines changed: 145 additions & 18 deletions

File tree

cpp/src/arrow/dataset/file_base.cc

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
#include "arrow/dataset/file_base.h"
1919

20+
#include "arrow/acero/accumulation_queue.h"
2021
#include "arrow/acero/exec_plan.h"
2122

2223
#include <algorithm>
@@ -559,13 +560,18 @@ Result<acero::ExecNode*> MakeWriteNode(acero::ExecPlan* plan,
559560
return node;
560561
}
561562

562-
class TeeNode : public acero::MapNode {
563+
class TeeNode : public acero::MapNode,
564+
public arrow::acero::util::SerialSequencingQueue::Processor {
563565
public:
564566
TeeNode(acero::ExecPlan* plan, std::vector<acero::ExecNode*> inputs,
565567
std::shared_ptr<Schema> output_schema,
566568
FileSystemDatasetWriteOptions write_options)
567569
: MapNode(plan, std::move(inputs), std::move(output_schema)),
568-
write_options_(std::move(write_options)) {}
570+
write_options_(std::move(write_options)) {
571+
if (write_options.preserve_order) {
572+
sequencer_ = acero::util::SerialSequencingQueue::Make(this);
573+
}
574+
}
569575

570576
Status StartProducing() override {
571577
ARROW_ASSIGN_OR_RAISE(
@@ -592,6 +598,28 @@ class TeeNode : public acero::MapNode {
592598

593599
const char* kind_name() const override { return "TeeNode"; }
594600

601+
Status Validate() const override {
602+
ARROW_RETURN_NOT_OK(acero::MapNode::Validate());
603+
if (inputs_[0]->ordering().is_unordered() && sequencer_) {
604+
return Status::Invalid("Tee node '", label(),
605+
"' is configured to sequence output but there is no "
606+
"meaningful ordering in the input");
607+
}
608+
return Status::OK();
609+
}
610+
611+
Status InputReceived(ExecNode* input, ExecBatch batch) override {
612+
DCHECK_EQ(input, inputs_[0]);
613+
if (sequencer_) {
614+
return sequencer_->InsertBatch(std::move(batch));
615+
}
616+
return Process(std::move(batch));
617+
}
618+
619+
Status Process(ExecBatch batch) override {
620+
return acero::MapNode::InputReceived(inputs_[0], batch);
621+
}
622+
595623
void Finish() override { dataset_writer_->Finish(); }
596624

597625
Result<compute::ExecBatch> ProcessBatch(compute::ExecBatch batch) override {
@@ -625,6 +653,7 @@ class TeeNode : public acero::MapNode {
625653
std::unique_ptr<internal::DatasetWriter> dataset_writer_;
626654
FileSystemDatasetWriteOptions write_options_;
627655
std::atomic<int32_t> backpressure_counter_ = 0;
656+
std::unique_ptr<acero::util::SerialSequencingQueue> sequencer_{nullptr};
628657
};
629658

630659
} // namespace

cpp/src/arrow/dataset/file_test.cc

Lines changed: 114 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#include <arrow/record_batch.h>
3333
#include <arrow/util/async_generator.h>
3434
#include "arrow/acero/exec_plan.h"
35+
#include "arrow/acero/test_nodes.h"
3536
#include "arrow/acero/test_util_internal.h"
3637
#include "arrow/array/array_primitive.h"
3738
#include "arrow/compute/test_util_internal.h"
@@ -44,6 +45,7 @@
4445
#include "arrow/filesystem/test_util.h"
4546
#include "arrow/status.h"
4647
#include "arrow/testing/future_util.h"
48+
#include "arrow/testing/generator.h"
4749
#include "arrow/testing/gtest_util.h"
4850
#include "arrow/util/io_util.h"
4951

@@ -444,6 +446,65 @@ class MockDataset : public Dataset {
444446
};
445447
};
446448

449+
constexpr random::SeedType kJitterSeed = 42;
450+
constexpr int kMaxJitterModifier = 4;
451+
constexpr int64_t kOrderingRowsPerBatch = 1;
452+
constexpr int kOrderingNumBatches = 256;
453+
454+
Result<bool> HasOutOfOrderRows(const Table& table) {
455+
TableBatchReader reader(table);
456+
std::shared_ptr<RecordBatch> batch;
457+
ARROW_RETURN_NOT_OK(reader.ReadNext(&batch));
458+
int32_t prev = 0;
459+
bool has_prev = false;
460+
while (batch != nullptr) {
461+
const auto* values = batch->column(0)->data()->GetValues<int32_t>(1);
462+
for (int row = 0; row < batch->num_rows(); ++row) {
463+
int32_t value = values[row];
464+
if (has_prev && value <= prev) {
465+
return true;
466+
}
467+
prev = value;
468+
has_prev = true;
469+
}
470+
ARROW_RETURN_NOT_OK(reader.ReadNext(&batch));
471+
}
472+
return false;
473+
}
474+
475+
TEST_F(TestFileSystemDataset, RejectPreserveOrderWithUnorderedInput) {
476+
dataset::internal::Initialize();
477+
478+
auto format = std::make_shared<IpcFileFormat>();
479+
FileSystemDatasetWriteOptions write_options;
480+
write_options.file_write_options = format->DefaultWriteOptions();
481+
write_options.filesystem = std::make_shared<fs::internal::MockFileSystem>(fs::kNoTime);
482+
write_options.base_dir = "root";
483+
write_options.partitioning = std::make_shared<HivePartitioning>(schema({}));
484+
write_options.basename_template = "{i}.feather";
485+
write_options.preserve_order = true;
486+
487+
auto source_data = acero::MakeBasicBatches();
488+
for (const char* factory_name : {"write", "tee"}) {
489+
SCOPED_TRACE(factory_name);
490+
ASSERT_OK_AND_ASSIGN(auto plan, acero::ExecPlan::Make());
491+
AsyncGenerator<std::optional<cp::ExecBatch>> sink_gen;
492+
std::vector<acero::Declaration> declarations = {
493+
{"source",
494+
acero::SourceNodeOptions{source_data.schema, source_data.gen(false, false)}},
495+
{factory_name, WriteNodeOptions{write_options}},
496+
};
497+
if (std::string(factory_name) == "tee") {
498+
declarations.emplace_back("sink", acero::SinkNodeOptions{&sink_gen});
499+
}
500+
ASSERT_OK(
501+
acero::Declaration::Sequence(std::move(declarations)).AddToPlan(plan.get()));
502+
ASSERT_THAT(plan->Validate(),
503+
Raises(StatusCode::Invalid,
504+
::testing::HasSubstr("no meaningful ordering in the input")));
505+
}
506+
}
507+
447508
TEST_F(TestFileSystemDataset, MultiThreadedWritePersistsOrder) {
448509
// Test for GH-26818
449510
//
@@ -458,6 +519,7 @@ TEST_F(TestFileSystemDataset, MultiThreadedWritePersistsOrder) {
458519
//
459520
// If this test starts to reliably fail with preserve_order == false, the test setup
460521
// has to be revised to again reliably produce out-of-order sequences.
522+
461523
auto format = std::make_shared<IpcFileFormat>();
462524
FileSystemDatasetWriteOptions write_options;
463525
write_options.file_write_options = format->DefaultWriteOptions();
@@ -500,26 +562,62 @@ TEST_F(TestFileSystemDataset, MultiThreadedWritePersistsOrder) {
500562
ASSERT_OK(scanner_builder->UseThreads(false));
501563
ASSERT_OK_AND_ASSIGN(scanner, scanner_builder->Finish());
502564
ASSERT_OK_AND_ASSIGN(auto actual, scanner->ToTable());
503-
TableBatchReader reader(*actual);
504-
std::shared_ptr<RecordBatch> batch;
505-
ASSERT_OK(reader.ReadNext(&batch));
506-
int32_t prev = -1;
507-
auto out_of_order = false;
508-
while (batch != nullptr) {
509-
const auto* values = batch->column(0)->data()->GetValues<int32_t>(1);
510-
for (int row = 0; row < batch->num_rows(); ++row) {
511-
int32_t value = values[row];
512-
if (value <= prev) {
513-
out_of_order = true;
514-
}
515-
prev = value;
516-
}
517-
ASSERT_OK(reader.ReadNext(&batch));
518-
}
565+
ASSERT_OK_AND_ASSIGN(auto out_of_order, HasOutOfOrderRows(*actual));
519566
ASSERT_EQ(!out_of_order, preserve_order);
520567
}
521568
}
522569

570+
TEST_F(TestFileSystemDataset, MultiThreadedTeeWritePersistsOrder) {
571+
dataset::internal::Initialize();
572+
acero::RegisterTestNodes();
573+
574+
auto format = std::make_shared<IpcFileFormat>();
575+
auto fs = std::make_shared<fs::internal::MockFileSystem>(fs::kNoTime);
576+
FileSystemDatasetWriteOptions write_options;
577+
write_options.file_write_options = format->DefaultWriteOptions();
578+
write_options.filesystem = fs;
579+
write_options.partitioning = std::make_shared<HivePartitioning>(schema({}));
580+
write_options.basename_template = "{i}.feather";
581+
582+
auto unordered_write_options = write_options;
583+
unordered_write_options.base_dir = "unordered";
584+
unordered_write_options.preserve_order = false;
585+
auto ordered_write_options = write_options;
586+
ordered_write_options.base_dir = "ordered";
587+
ordered_write_options.preserve_order = true;
588+
589+
auto input = gen::Gen({gen::Step<int32_t>()})
590+
->FailOnError()
591+
->Table(kOrderingRowsPerBatch, kOrderingNumBatches);
592+
593+
// The first TeeNode records the jittered, out-of-order stream without changing it.
594+
// The second TeeNode must use the batch indices to restore order.
595+
ASSERT_OK(acero::DeclarationToStatus(acero::Declaration::Sequence(
596+
{{"table_source", acero::TableSourceNodeOptions{input}},
597+
{"jitter", acero::JitterNodeOptions{kJitterSeed, kMaxJitterModifier}},
598+
{"tee", WriteNodeOptions{unordered_write_options}, "unordered_tee"},
599+
{"tee", WriteNodeOptions{ordered_write_options}, "ordered_tee"}})));
600+
601+
auto read_written_table =
602+
[&](const std::string& path) -> Result<std::shared_ptr<Table>> {
603+
ARROW_ASSIGN_OR_RAISE(auto dataset_factory,
604+
FileSystemDatasetFactory::Make(fs, {path}, format, {}));
605+
ARROW_ASSIGN_OR_RAISE(auto written_dataset, dataset_factory->Finish(FinishOptions{}));
606+
ARROW_ASSIGN_OR_RAISE(auto written_scanner_builder, written_dataset->NewScan());
607+
ARROW_RETURN_NOT_OK(written_scanner_builder->UseThreads(false));
608+
ARROW_ASSIGN_OR_RAISE(auto written_scanner, written_scanner_builder->Finish());
609+
return written_scanner->ToTable();
610+
};
611+
612+
ASSERT_OK_AND_ASSIGN(auto unordered_table, read_written_table("unordered/0.feather"));
613+
ASSERT_OK_AND_ASSIGN(auto unordered_out_of_order, HasOutOfOrderRows(*unordered_table));
614+
ASSERT_TRUE(unordered_out_of_order);
615+
616+
ASSERT_OK_AND_ASSIGN(auto ordered_table, read_written_table("ordered/0.feather"));
617+
ASSERT_OK_AND_ASSIGN(auto ordered_out_of_order, HasOutOfOrderRows(*ordered_table));
618+
ASSERT_FALSE(ordered_out_of_order);
619+
}
620+
523621
class FileSystemWriteTest : public testing::TestWithParam<std::tuple<bool, bool>> {
524622
using PlanFactory = std::function<std::vector<acero::Declaration>(
525623
const FileSystemDatasetWriteOptions&,

0 commit comments

Comments
 (0)