Skip to content

"Partition boost" the group by queries in MSQ for better splits#15474

Merged
LakshSingla merged 3 commits intoapache:masterfrom
LakshSingla:groupby-boost
Jan 11, 2024
Merged

"Partition boost" the group by queries in MSQ for better splits#15474
LakshSingla merged 3 commits intoapache:masterfrom
LakshSingla:groupby-boost

Conversation

@LakshSingla
Copy link
Contributor

Description

In MSQ, each partition corresponds to a single segment (if it is the end stage) or the unit of work that is assigned to a worker. If the clustering key has low cardinality, then the returned partitions can be too large. For certain stages, we can break up the resulting partitions by introducing a synthetic clustering column "__boost" (which should contain a unique value - incrementally increasing longs for MSQ's implementation) at the end of the clustering keys, so that the keys become unique, and the partition boundaries are respectable. This is done in ScanQueryKit, and can be done in the GroupByPostShuffleFrameProcessor.

This PR introduces this partition boosting to the group by queries as well.

For example, the queries of the form

INSERT INTO table
SELECT dim1, dim2, agg(metric1) as aggregated FROM EXTERN(...)
GROUP BY dim1, dim2
PARTITIONED BY YEAR
CLUSTERED BY aggregated

can run into large segments if the "aggregated" is a very low cardinality column, and the (dim1, dim2) is a high cardinality pair. The solution (before the patch) would be to add more dims into the CLUSTERED BY clause to make it more unique. Post this patch, that won't be required.
ControllerImpl already handles partition-boosted columns specially.

NOTE: Group by queries can fail during the cluster upgrade, if some of the workers are on an older version, while the others are on a newer version.

Release note


Key changed/added classes in this PR
  • MyFoo
  • OurBar
  • TheirBaz

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@github-actions github-actions bot added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Dec 3, 2023
final GroupByQuery query
)
{
List<VirtualColumn> virtualColumns = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

We need a UT for this.

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 removed the conditionals all together, therefore the code is now straightforward.

Copy link
Contributor

@cryptoe cryptoe left a comment

Choose a reason for hiding this comment

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

Lets add some test cases. We do not want the boosted column as part of the segment.


if (frameWriter.addSelection()) {
if (partitionBoostVirtualColumn != null) {
partitionBoostVirtualColumn.setValue(partitionBoostVirtualColumn.getValue() + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this cause the compare function on L161 to behave weirdly ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compare function won't be behaving weirdly because:

  1. The boosted column is only a part of the final written frame. The outpuRow doesn't contain the boosted column
  2. Also, the compare function is computed based on the group by query, which will compare the dims and the __time column (if needed). Since __boost doesn't come there, it will choose to ignore the column altogether.

This can be confirmed in the COUNT(*) tests testInsertOnExternalDataSource which aggregates the rows with the correct result.

resultClusterByWithPartitionBoostColumns.add(new KeyColumn(QueryKitUtils.PARTITION_BOOST_COLUMN, KeyOrder.ASCENDING));
ClusterBy resultClusterByWithPartitionBoost = new ClusterBy(
resultClusterByWithPartitionBoostColumns,
resultClusterByWithoutPartitionBoost.getBucketByCount()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think line 153 : 167 can be another method called createRowSignature.
Also the if on line 159 should be the first switch in the control flow. Something like

SigX=null; 
if(boosted)
{
sigX=foo
}else{
sigX=bar
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored to different function, and cleaned up the main code flow.

@LakshSingla
Copy link
Contributor Author

We do not want the boosted column as part of the segment.

This is the job of the controller to remove the boosted column. Also, we have pre-existing MSQInsertTests, which assert on the expected row and the row signature of the segment. The boosted column doesn't show up there.

@LakshSingla
Copy link
Contributor Author

I have also removed the conditional in the GroupByFrameProcessor and its factory because depending on the signature passed by the query kit, it will choose to add or ignore the partition boosted column. If the signature has __boost column in it, it will pick the column from the selector factory. Else, it will ignore the virtual column altogether. There's no need for additional conditional in the processor or the factory.

@LakshSingla
Copy link
Contributor Author

Screenshot 2024-01-09 at 3 11 32 PM
Tested that there's a __boost column at the end of the post-processor factory. Also, I went into the debug mode into the GroupByPostShuffleFrameProcessor and the SegmentGeneratorFrameProcessor and the boost column is present in both places with an incrementing value.

Copy link
Contributor

@cryptoe cryptoe left a comment

Choose a reason for hiding this comment

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

Nit comments.
LGTM.

@LakshSingla
Copy link
Contributor Author

Thanks for the review @cryptoe

@LakshSingla LakshSingla merged commit 87fbe42 into apache:master Jan 11, 2024
@LakshSingla LakshSingla deleted the groupby-boost branch January 11, 2024 07:16
@LakshSingla LakshSingla added this to the 29.0.0 milestone Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants