Skip to content
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

Core: Fix performance issue when combining tasks by partition #9629

Merged
merged 1 commit into from
Feb 3, 2024

Conversation

aokolnychyi
Copy link
Contributor

@aokolnychyi aokolnychyi commented Feb 2, 2024

This PR fixes a substantial performance issue in combining tasks by partition for SPJ, which was caused by repetitive creation of PartitionData, which triggered expensive initialization.

Before

Benchmark                                                    Mode  Cnt   Score   Error  Units
TaskGroupPlanningBenchmark.planTaskGroups                      ss    5   2.879 ± 0.209   s/op
TaskGroupPlanningBenchmark.planTaskGroups:async                ss          NaN            ---
TaskGroupPlanningBenchmark.planTaskGroupsWithGrouping          ss    5  26.564 ± 0.410   s/op
TaskGroupPlanningBenchmark.planTaskGroupsWithGrouping:async    ss          NaN            ---

After

Benchmark                                                    Mode  Cnt  Score   Error  Units
TaskGroupPlanningBenchmark.planTaskGroups                      ss    5  2.830 ± 0.211   s/op
TaskGroupPlanningBenchmark.planTaskGroups:async                ss         NaN            ---
TaskGroupPlanningBenchmark.planTaskGroupsWithGrouping          ss    5  5.167 ± 0.464   s/op
TaskGroupPlanningBenchmark.planTaskGroupsWithGrouping:async    ss         NaN            ---

@aokolnychyi aokolnychyi added this to the Iceberg 1.5.0 milestone Feb 2, 2024
return data;
}

private static Object toInternalValue(Object value) {
Copy link
Contributor Author

@aokolnychyi aokolnychyi Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved from set to avoid calling that public method in the new constructor.

Types.StructType groupingKeyType,
StructLike partition) {

PartitionData groupingKey = new PartitionData(groupingKeyType);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was causing the issue.

Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar Feb 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my understanding, the repeated calling of this constructor was expensive because the repeated data array initialization (and possibly the determining of the partitionType via AvroSchemaUtil.convert)? And now we avoid repeatedly performing those computations by having a PartitionData be initialized with a "template" just once for all tasks and then replace the actual data in PartitionData#copyFor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, correct, @amogh-jahagirdar.

@@ -171,6 +169,10 @@ public PartitionData copy() {
return new PartitionData(this);
}

public PartitionData copyFor(StructLike partition) {
Copy link
Contributor Author

@aokolnychyi aokolnychyi Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative to this is to create a new StructLike class simply backed by an array and populate it in the table scan utility. I went with this approach because we do a similar trick in other places.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this API to be a bit awkward. I'd rather have a more straightforward two-step process to copy the key without copying values (like emptyCopy()) and then fill that with values. Otherwise "copy" doesn't really describe this action, which copies behavior but not data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not entirely happy too. I followed what we did in other places like StructLikeProjection.

Do we really need PartitionData here? What if we simply create a container struct backed by an array and use it?

My worry with emptyCopy() is that the result object is in a weird state, I am not sure it is actually better than copyFor().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need to a serializable projection used as a grouping key. Not more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative to this is to create a new StructLike class simply backed by an array and populate it in the table scan utility

There's already org.apache.iceberg.io.StructCopy class, maybe we can leverage that.

@aokolnychyi aokolnychyi force-pushed the fix-table-scan-util-perf branch from b1a90b0 to a52019e Compare February 2, 2024 23:22
@rdblue rdblue merged commit fb02bd2 into apache:main Feb 3, 2024
42 checks passed
@rdblue
Copy link
Contributor

rdblue commented Feb 3, 2024

Thanks for the fix, @aokolnychyi! I think it's important to get this into 1.5 so I merged this. The method name should be okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants