Fail MSQ compaction if multi-valued partition dimensions are found #17344

gargvishesh · 2024-10-15T05:07:34Z

MSQ currently supports only single-valued string dimensions as partition keys. This patch adds a check to ensure that partition keys are single-valued in case this info is available by virtue of segment download for schema inference.

During compaction, if MSQ finds multi-valued dimensions (MVDs) declared as part of range partitionsSpec, it switches partitioning type to dynamic, ending up in repeated compactions of the same interval. To avoid this scenario, the segment download logic is also updated to always download segments if info on multi-valued dimensions is required.

…ensions

kfaraz · 2024-10-15T05:16:36Z

the partitioning eventually ends up getting switched to dynamic type.

@gargvishesh , is this something MSQ always does silently?

...-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQCompactionRunner.java

server/src/main/java/org/apache/druid/client/indexing/ClientCompactionRunnerInfo.java

gargvishesh · 2024-10-15T07:46:37Z

@kfaraz: yes, if MVD fields are found, there is just a log that states shard spec is changed to NumberedShardSpec type:

druid/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java

Line 1104 in c2149d5

log.info("ShardSpec chosen: %s", commentary);

This in fact also means that compaction will keeping continuing in a loop. I was thinking about looking for a solution to this problem post this PR.

kfaraz · 2024-10-15T07:57:20Z

This in fact also means that compaction will keeping continuing in a loop. I was thinking about looking for a solution to this problem post this PR.

Oh, that can be a problem. When can this happen?
Once we have downloaded the segments, we know if it has multi-valued dimensions or not. So the compaction task would always fail, correct?

gargvishesh · 2024-10-15T08:00:22Z

Right. So this case can happen when there is no need to download the segments, ie. user specifies dimensions, metrics, granularity and rollup in the compaction spec itself, but one of the dimensions happens to be MVD.

kfaraz · 2024-10-15T08:04:01Z

Can we ensure we download the segments in this case too?
What would be the drawback?

gargvishesh · 2024-10-15T08:15:57Z

Drawback was mainly the high cost of downloading segments. But yes I think this would be the only viable way for now.

gargvishesh · 2024-10-16T08:23:54Z

@kfaraz: I have now updated the logic to download segments when we need MVD info. The check is also updated to be more fine grained to avoid redundant download.

kfaraz · 2024-10-16T08:29:27Z

indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java

+      // Segments are downloaded even when just needMultiValuedDimensions=true since MSQ switches to dynamic
+      // partitioning on finding any 'range' partition dimension to be multi-valued at runtime, which ends up in a
+      // mismatch between the compaction config and the actual segments (lastCompactionState), leading to
+      // repeated compactions.


Please put this in the javadoc of this method instead.

kfaraz · 2024-10-16T13:10:14Z

indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java

+  static boolean isPossiblyRollup(@Nullable ClientCompactionTaskGranularitySpec granularitySpec)
+  {
+    return !(granularitySpec != null && Boolean.FALSE.equals(granularitySpec.isRollup()));
+  }
+
+  static boolean isRangePartitioned(@Nullable TuningConfig tuningConfig)
+  {
+    return tuningConfig != null && tuningConfig.getPartitionsSpec() instanceof DimensionRangePartitionsSpec;
+  }
+
+  static boolean isRollupOnStringDimension(
+      DimensionsSpec dimensionsSpec,
+      @Nullable ClientCompactionTaskGranularitySpec granularitySpec
+  )
+  {
+    if (!isPossiblyRollup(granularitySpec)) {
+      return false;
+    }
+    return dimensionsSpec.getDimensions()
+                         .stream()
+                         .anyMatch(dim -> ColumnType.STRING.equals(dim.getColumnType()));
+  }
+
+  static boolean isPartitionedOnStringDimension(DimensionsSpec dimensionsSpec, @Nullable TuningConfig tuningConfig)


I don't think all of these need to be separate methods as they are not being reused anywhere afaict. They can just be variables inside the needMVD method.

kfaraz · 2024-10-17T04:41:08Z

indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java

+   * therefore is particularly for MSQCompactionRunner; it returns true when dimension types aren't known
+   * from CompactionTask spec, or if either rollup or partition dimensions contain any string-type column.
+   */
+  public static boolean needMultiValuedDimensions(CompactionTask compactionTask)


No point making this method static if it requires passing the CompactionTask into it.
Also, instead of making this method public or package private, CompactionTask should instead expose a method needsToDownloadSegments() or something. That can internally call this one.

No point making this method static if it requires passing the CompactionTask into it.

Agree. One possibility is that its value can be assigned to a final field and the getter is exposed

Also, instead of making this method public or package private, CompactionTask should instead expose a method needsToDownloadSegments() or something. That can internally call this one.

This would need some refactoring since the effective granularitySpec on which download depends is computed inside createDataSchemaForIntervals, just before passing this computed value to ExistingSegmentAnalyzer where the final need to download is evaluated. Also, each need* flag is separately communicated to the Analyzer class to do selective processing.

Does the proposed approach of assigning needMultiValuedDimensions to a private field and creating tests on it work? In fact, all such fields apart from needGranularitySpec can be populated in CompactionTask constructor and passed to the Analyzer

Just relooked at code. Only segmentGranularity is computed later, so all need* flags and an additional new needsDownload flag can be computed in the CompactionTask constructor itself. The needsDownload can have a public getter for tests.

If only needed in test, keep the method as package private rather than public.

public is needed since the tests reside in the MSQ extension

kfaraz

Approach looks good. Minor cleanup suggestions.

indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java

kfaraz · 2024-10-18T05:42:33Z

indexing-service/src/test/java/org/apache/druid/indexing/common/task/CompactionTaskTest.java

@@ -2015,6 +2100,37 @@ public SettableColumnValueSelector<?> makeNewSettableColumnValueSelector()

  }

+  /**
+   * A class to mimic MSQCompactionRunner behaviour, since the actual class resides in the MSQ extn


How are we mimicking the behaviour? The methods seem to return only null.

Just mimics for validations. Updated the comment

...-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQCompactionRunner.java

…pache#17344) MSQ currently supports only single-valued string dimensions as partition keys. This patch adds a check to ensure that partition keys are single-valued in case this info is available by virtue of segment download for schema inference. During compaction, if MSQ finds multi-valued dimensions (MVDs) declared as part of `range` partitionsSpec, it switches partitioning type to dynamic, ending up in repeated compactions of the same interval. To avoid this scenario, the segment download logic is also updated to always download segments if info on multi-valued dimensions is required.

Add multi-valued check for partition dimensions

ec79357

github-actions bot added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Oct 15, 2024

Merge branch 'refs/heads/master' into add-mvd-check-for-partition-dim…

112157a

…ensions

kfaraz reviewed Oct 15, 2024

View reviewed changes

gargvishesh changed the title ~~Add multi-valued check for partition dimensions~~ Fail MSQ compaction if multi-valued partition dimensions are found Oct 15, 2024

gargvishesh added 2 commits October 15, 2024 16:15

Address comments

97e4d21

Download segments if needMultiValuedDimensions

ec94741

github-actions bot added the Area - Ingestion label Oct 16, 2024

kfaraz reviewed Oct 17, 2024

View reviewed changes

gargvishesh added 3 commits October 17, 2024 19:01

Address review comment 2 and add tests to fix coverage

a503a47

Fix style

473c03f

Move multiValued check to MSQCompactionRunner itself

374b6ef

kfaraz approved these changes Oct 18, 2024

View reviewed changes

Fix formatting based on comments

efc5140

kfaraz approved these changes Oct 18, 2024

View reviewed changes

kfaraz merged commit 5da9949 into apache:master Oct 19, 2024
93 of 94 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fail MSQ compaction if multi-valued partition dimensions are found #17344

Fail MSQ compaction if multi-valued partition dimensions are found #17344

gargvishesh commented Oct 15, 2024 •

edited

Loading

kfaraz commented Oct 15, 2024

gargvishesh commented Oct 15, 2024

kfaraz commented Oct 15, 2024 •

edited

Loading

gargvishesh commented Oct 15, 2024

kfaraz commented Oct 15, 2024

gargvishesh commented Oct 15, 2024

gargvishesh commented Oct 16, 2024

kfaraz Oct 16, 2024

kfaraz Oct 16, 2024

kfaraz Oct 17, 2024

gargvishesh Oct 17, 2024

gargvishesh Oct 17, 2024

kfaraz Oct 17, 2024

gargvishesh Oct 17, 2024

kfaraz left a comment

kfaraz Oct 18, 2024

gargvishesh Oct 18, 2024

Fail MSQ compaction if multi-valued partition dimensions are found #17344

Fail MSQ compaction if multi-valued partition dimensions are found #17344

Conversation

gargvishesh commented Oct 15, 2024 • edited Loading

kfaraz commented Oct 15, 2024

gargvishesh commented Oct 15, 2024

kfaraz commented Oct 15, 2024 • edited Loading

gargvishesh commented Oct 15, 2024

kfaraz commented Oct 15, 2024

gargvishesh commented Oct 15, 2024

gargvishesh commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfaraz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gargvishesh commented Oct 15, 2024 •

edited

Loading

kfaraz commented Oct 15, 2024 •

edited

Loading