-
Notifications
You must be signed in to change notification settings - Fork 44
feat(agg): support paimon Java config aggregation.remove-record-on-delete #225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -49,13 +49,29 @@ Result<std::unique_ptr<AggregateMergeFunction>> AggregateMergeFunction::Create( | |||||||||||||||||||||||||||||||||||||||||
| aggregators.push_back(std::move(agg)); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| bool remove_record_on_delete = options.AggregationRemoveRecordOnDelete(); | ||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add check in class FieldAggregatorFactory like Java Paimon,
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| PAIMON_ASSIGN_OR_RAISE(std::vector<InternalRow::FieldGetterFunc> getters, | ||||||||||||||||||||||||||||||||||||||||||
| InternalRowUtils::CreateFieldGetters(value_schema, /*use_view=*/true)); | ||||||||||||||||||||||||||||||||||||||||||
| return std::unique_ptr<AggregateMergeFunction>( | ||||||||||||||||||||||||||||||||||||||||||
| new AggregateMergeFunction(std::move(getters), std::move(aggregators))); | ||||||||||||||||||||||||||||||||||||||||||
| return std::unique_ptr<AggregateMergeFunction>(new AggregateMergeFunction( | ||||||||||||||||||||||||||||||||||||||||||
| std::move(getters), std::move(aggregators), remove_record_on_delete)); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| Status AggregateMergeFunction::Add(KeyValue&& kv) { | ||||||||||||||||||||||||||||||||||||||||||
| // When removeRecordOnDelete is enabled, if we receive a DELETE row, | ||||||||||||||||||||||||||||||||||||||||||
| // mark the current row for deletion and initialize the row with input values. | ||||||||||||||||||||||||||||||||||||||||||
| if (remove_record_on_delete_ && kv.value_kind == RowKind::Delete()) { | ||||||||||||||||||||||||||||||||||||||||||
| current_delete_row_ = true; | ||||||||||||||||||||||||||||||||||||||||||
| row_ = std::make_unique<GenericRow>(getters_.size()); | ||||||||||||||||||||||||||||||||||||||||||
| for (size_t i = 0; i < getters_.size(); i++) { | ||||||||||||||||||||||||||||||||||||||||||
| row_->SetField(i, getters_[i](*(kv.value))); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| row_->AddDataHolder(std::move(kv.value)); | ||||||||||||||||||||||||||||||||||||||||||
| latest_kv_ = std::move(kv); | ||||||||||||||||||||||||||||||||||||||||||
| return Status::OK(); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| current_delete_row_ = false; | ||||||||||||||||||||||||||||||||||||||||||
| bool is_retract = kv.value_kind->IsRetract(); | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+73
to
75
|
||||||||||||||||||||||||||||||||||||||||||
| current_delete_row_ = false; | |
| bool is_retract = kv.value_kind->IsRetract(); | |
| bool was_delete_row = current_delete_row_; | |
| current_delete_row_ = false; | |
| bool is_retract = kv.value_kind->IsRetract(); | |
| // If the previous state was a delete row produced by removeRecordOnDelete, | |
| // a following add record should start a new accumulator instead of merging | |
| // with the deleted row's values. | |
| if (was_delete_row && !is_retract) { | |
| row_ = std::make_unique<GenericRow>(getters_.size()); | |
| for (size_t i = 0; i < getters_.size(); i++) { | |
| row_->SetField(i, getters_[i](*(kv.value))); | |
| } | |
| row_->AddDataHolder(std::move(kv.value)); | |
| latest_kv_ = std::move(kv); | |
| return Status::OK(); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think this is a good suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will not happen delete followed by re-insert
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -191,4 +191,109 @@ TEST(AggregateMergeFunctionTest, TestSequenceFields) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| KeyValueChecker::CheckResult(expected, result_kv, /*key_arity=*/1, /*value_arity=*/4); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| TEST(AggregateMergeFunctionTest, TestRemoveRecordOnDelete) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| arrow::FieldVector fields = {arrow::field("k0", arrow::int32()), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| arrow::field("v0", arrow::int32())}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| auto value_schema = arrow::schema(fields); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ASSERT_OK_AND_ASSIGN( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CoreOptions core_options, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CoreOptions::FromMap({{Options::FIELDS_DEFAULT_AGG_FUNC, "sum"}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| {Options::AGGREGATION_REMOVE_RECORD_ON_DELETE, "true"}})); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ASSERT_OK_AND_ASSIGN( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| std::unique_ptr<AggregateMergeFunction> merge_func, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AggregateMergeFunction::Create(value_schema, /*primary_keys=*/{"k0"}, core_options)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| auto pool = GetDefaultPool(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Case 1: INSERT + INSERT, then DELETE -> result should be RowKind::Delete | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| merge_func->Reset(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| KeyValue kv1(RowKind::Insert(), /*sequence_number=*/0, /*level=*/0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10}, pool.get()), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10, 100}, pool.get())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| KeyValue kv2(RowKind::Insert(), /*sequence_number=*/1, /*level=*/0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10}, pool.get()), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10, 200}, pool.get())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| KeyValue kv3(RowKind::Delete(), /*sequence_number=*/2, /*level=*/0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10}, pool.get()), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10, 300}, pool.get())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ASSERT_OK(merge_func->Add(std::move(kv1))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ASSERT_OK(merge_func->Add(std::move(kv2))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ASSERT_OK(merge_func->Add(std::move(kv3))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| auto result_kv = std::move(merge_func->GetResult().value().value()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Should return DELETE row kind with the original values from the delete record | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| KeyValue expected(RowKind::Delete(), /*sequence_number=*/2, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /*level=*/KeyValue::UNKNOWN_LEVEL, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10}, pool.get()), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10, 300}, pool.get())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| KeyValueChecker::CheckResult(expected, result_kv, /*key_arity=*/1, /*value_arity=*/2); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Case 2: Only INSERT rows, no DELETE -> result should be RowKind::Insert with aggregated | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // values | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| merge_func->Reset(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| KeyValue kv1(RowKind::Insert(), /*sequence_number=*/0, /*level=*/0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10}, pool.get()), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10, 100}, pool.get())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| KeyValue kv2(RowKind::Insert(), /*sequence_number=*/1, /*level=*/0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10}, pool.get()), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10, 200}, pool.get())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ASSERT_OK(merge_func->Add(std::move(kv1))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ASSERT_OK(merge_func->Add(std::move(kv2))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| auto result_kv = std::move(merge_func->GetResult().value().value()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Should return INSERT with sum aggregation: 100 + 200 = 300 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| KeyValue expected(RowKind::Insert(), /*sequence_number=*/1, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /*level=*/KeyValue::UNKNOWN_LEVEL, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10}, pool.get()), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10, 300}, pool.get())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| KeyValueChecker::CheckResult(expected, result_kv, /*key_arity=*/1, /*value_arity=*/2); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Case 3: DELETE only -> result should be RowKind::Delete | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| merge_func->Reset(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| KeyValue kv1(RowKind::Delete(), /*sequence_number=*/0, /*level=*/0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10}, pool.get()), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10, 100}, pool.get())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ASSERT_OK(merge_func->Add(std::move(kv1))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| auto result_kv = std::move(merge_func->GetResult().value().value()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| KeyValue expected(RowKind::Delete(), /*sequence_number=*/0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /*level=*/KeyValue::UNKNOWN_LEVEL, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10}, pool.get()), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BinaryRowGenerator::GenerateRowPtr({10, 100}, pool.get())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| KeyValueChecker::CheckResult(expected, result_kv, /*key_arity=*/1, /*value_arity=*/2); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | |
| } | |
| // Case 4: DELETE followed by higher-sequence INSERT should start from a fresh | |
| // accumulator when removeRecordOnDelete is enabled. | |
| { | |
| merge_func->Reset(); | |
| KeyValue kv1(RowKind::Insert(), /*sequence_number=*/0, /*level=*/0, | |
| BinaryRowGenerator::GenerateRowPtr({10}, pool.get()), | |
| BinaryRowGenerator::GenerateRowPtr({10, 100}, pool.get())); | |
| KeyValue kv2(RowKind::Delete(), /*sequence_number=*/1, /*level=*/0, | |
| BinaryRowGenerator::GenerateRowPtr({10}, pool.get()), | |
| BinaryRowGenerator::GenerateRowPtr({10, 200}, pool.get())); | |
| KeyValue kv3(RowKind::Insert(), /*sequence_number=*/2, /*level=*/0, | |
| BinaryRowGenerator::GenerateRowPtr({10}, pool.get()), | |
| BinaryRowGenerator::GenerateRowPtr({10, 300}, pool.get())); | |
| ASSERT_OK(merge_func->Add(std::move(kv1))); | |
| ASSERT_OK(merge_func->Add(std::move(kv2))); | |
| ASSERT_OK(merge_func->Add(std::move(kv3))); | |
| auto result_kv = std::move(merge_func->GetResult().value().value()); | |
| KeyValue expected(RowKind::Insert(), /*sequence_number=*/2, | |
| /*level=*/KeyValue::UNKNOWN_LEVEL, | |
| BinaryRowGenerator::GenerateRowPtr({10}, pool.get()), | |
| BinaryRowGenerator::GenerateRowPtr({10, 300}, pool.get())); | |
| KeyValueChecker::CheckResult(expected, result_kv, /*key_arity=*/1, /*value_arity=*/2); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also not correct.
Paimon has its own snapshot
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,4 +114,14 @@ TEST(FieldAggregatorFactoryTest, TestSimple) { | |
| } | ||
| } | ||
|
|
||
| TEST(FieldAggregatorFactoryTest, TestRemoveRecordOnDeleteConflictsWithIgnoreRetract) { | ||
| ASSERT_OK_AND_ASSIGN( | ||
| CoreOptions options, | ||
| CoreOptions::FromMap({{Options::AGGREGATION_REMOVE_RECORD_ON_DELETE, "true"}, | ||
| {"fields.f0.ignore-retract", "true"}})); | ||
| auto result = FieldAggregatorFactory::CreateFieldAggregator("f0", arrow::int32(), "sum", options); | ||
| ASSERT_FALSE(result.ok()); | ||
| ASSERT_TRUE(result.status().message().find("conflicting behavior") != std::string::npos); | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider use |
||
|
|
||
| } // namespace paimon::test | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add case in core_options_test.cpp for AggregationRemoveRecordOnDelete() api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CoreOptionsTest, TestDefaultValueandCoreOptionsTest, TestFromMapThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated