[MSQ] Confirm that the allocated segment's granularity matches the requested granularity by LakshSingla · Pull Request #14475 · apache/druid

LakshSingla · 2023-06-23T01:58:05Z

Description

Currently, if you run the two queries like below:

1. insert into mytest select time_parse(ts) as __time, c1 from (values('2022-01-01 00:00:00', 'A')) as t(ts, c1) partitioned by MONTH

2. insert into mytest select time_parse(ts) as __time, c1 from (values('2022-01-02 00:00:00', 'A')) as t(ts, c1) partitioned by DAY

The second one allocates a segment with MONTH granularity, even though the partitioning has stated DAY granularity.

This is because the overlord does a best-effort job of fetching the segment with the requested granularity and in case a pre-existing segment is there that overlaps the requested segment, the overlord returns that segment (which might not match the requested granularity).

This is semantically incorrect since the user has requested a segment of DAY granularity and this should return an error instead. This PR adds the desired checks to prevent the described case from happening.

Release notes

A regression has been fixed in MSQ which can cause INSERT to allocate segments of incorrect granularity in case pre-existing segments of coarser granularity are present. Such cases now fail with the InsertCannotAllocateSegment fault with an appropriate actionable error message.

This PR has:

kfaraz · 2023-06-23T16:43:28Z

+   * Note: ALL granularity isn't aligned to any interval, however this method is defines that ALL granularity matches
+   * an interval with boundary ({@code DateTimes.MIN}, {@code DateTimes.MAX})
+   */
+  public static boolean doesIntervalMatchesGranularity(final Interval interval, final Granularity granularity)


Nit: align probably suits better.

Suggested change

public static boolean doesIntervalMatchesGranularity(final Interval interval, final Granularity granularity)

public static boolean isAlignedWithGranularity(final Interval interval, final Granularity granularity)

Left a comment on the alternative implementation, which suggests to me that aligned is probably a misnomer as we want both the following conditions to hold true:

The interval is aligned with the granularity

The interval should fit exactly one "unit" of the granularity.

In case that comment holds true, we shouldn't name it aligned. I am open to alternatives as matches isn't very descriptive either 😥

kfaraz · 2023-06-23T17:08:42Z

    return retVal;
  }
+
+  /**


Maybe add this method to Intervals class instead, as it could have uses outside the MSQ extension too.

Left this method in the interval utils since the top-level Javadoc for IntervalUtils mentions that the methods are specific to MSQ for now, and would make more sense to move to Intervals if MSQ wasn't an extension

kfaraz · 2023-06-23T17:11:44Z

+      // the requested segment is present and that segment completely overlaps the request. Throw an error if the interval
+      // doesn't match the granularity requested
+      if (allocation == null
+          || !IntervalUtils.doesIntervalMatchesGranularity(allocation.getInterval(), segmentGranularity)) {


I think the error message should be different in the null case and the unexpected interval case.

Noted! I think we can make a new fault class then and signify to the user the pre-existing segment so that the user can use REPLACE, change the granularity in the query or not insert in the given time interval

Yeah, that would be nice!

kfaraz · 2023-06-23T17:52:15Z

+      return (interval.getStartMillis() == DateTimes.MIN.getMillis())
+             && (interval.getEndMillis() == DateTimes.MAX.getMillis());
+    }
+    return granularity.isAligned(interval) && granularity.bucketEnd(interval.getStart()).equals(interval.getEnd());


I think just isAligned should be enough. This is the code for PeriodGranularity.isAligned:

@Override public boolean isAligned(Interval interval) { return bucket(interval.getStart()).equals(interval); }

The above suggestion won't hold true in case the interval is a multiple of the granularity. For example an interval like doesIntervalMatchesGranularity("2022-01-01/2022-01-03", Granularities.MONTH) would return true with granularity.isAligned(), however false with the current implementation which is the desired behavior.

If we specify PARTITIONED BY DAY, we shouldn't allow an allocation of 2 days, therefore we check the partition end as well.

WDYT?

Yeah, but Granularity.isAligned already does just that. You can verify it by doing this:

Assert.assertTrue(Granularities.DAY.isAligned(Intervals.of("2011-01-01/2011-01-02"))); Assert.assertFalse(Granularities.DAY.isAligned(Intervals.of("2011-01-01/2011-01-03")));

In fact, it would be nice if you could include this test in this PR so that future devs are not confused.

You probably find the name isAligned a little confusing for this purpose. But if you think about it, here alignment doesn't mean that the start and end of the interval should be aligned with the cut marks of this granularity. Rather, it means that the interval must exactly fit into the scheme of this granularity. The javadoc also says something to this effect:

/** * Return true if time chunks populated by this granularity includes the given interval time chunk. */ public abstract boolean isAligned(Interval interval);

We can update this javadoc to say "Returns true only if time chunks ..." to reduce confusion.

Thanks for the clarification, I understand why isAligned checks that the interval fits exactly into the granularity. Refactored

LakshSingla · 2023-06-26T04:47:33Z

Thanks for the review!
Updated the PR with a new fault class, and a better user-facing error message along with the minor refactors mentioned

kfaraz

Minor nitpicks, otherwise changes look good.

kfaraz · 2023-06-26T06:50:14Z

+   * This is used to check if the granularity allocation made by the overlord is the same as the one requested in the
+   * SQL query
+   */
+  public static boolean isEternityOrDoesIntervalAlignWithGranularity(


Nit: you need not mention the part about eternity as an eternity interval does satisfy the definition of alignment with AllGranularity.

So this method can just be called isAligned or something to reflect the underlying usage of granularity.isAligned.

kfaraz · 2023-06-26T06:51:03Z

+      final Granularity granularity
+  )
+  {
+    // AllGranularity needs special handling since AllGranularity#bucketStart always returns false


I suppose we could fix AllGranularity.isAligned() to return true when the interval is eternity.

I was not confident if that would break any pre-existing logic. Also, Javadoc states:

/** * No interval is aligned with all granularity since it's infinite. */

Therefore I found suitable to extract the logic as a helper method

kfaraz · 2023-06-26T06:57:47Z

 | <a name="error_ColumnNameRestricted">`ColumnNameRestricted`</a> | The query uses a restricted column name. | `columnName`: The restricted column name. |
 | <a name="error_ColumnTypeNotSupported">`ColumnTypeNotSupported`</a> | The column type is not supported. This can be because:<br /> <br /><ul><li>Support for writing or reading from a particular column type is not supported.</li><li>The query attempted to use a column type that is not supported by the frame format. This occurs with ARRAY types, which are not yet implemented for frames.</li></ul> | `columnName`: The column name with an unsupported type.<br /> <br />`columnType`: The unknown column type. |
 | <a name="error_InsertCannotAllocateSegment">`InsertCannotAllocateSegment`</a> | The controller task could not allocate a new segment ID due to conflict with existing segments or pending segments. Common reasons for such conflicts:<br /> <br /><ul><li>Attempting to mix different granularities in the same intervals of the same datasource.</li><li>Prior ingestions that used non-extendable shard specs.</li></ul>| `dataSource`<br /> <br />`interval`: The interval for the attempted new segment allocation. |
+| <a name="error_InsertCannotAllocateSegment">`InsertAllocatedIncorrectSegment`</a> | The controller task could not allocate a new segment ID of the specified granularity due to conflict with existing segments or pending segments. <br /> <br /> This happens when a coarser segment is already overlapping the interval for which the allocation was requested. Either use a REPLACE to overwrite over the existing overlapping segment or re-run INSERT with the pre-existing segment granularity in order to append to the interval| `dataSource`<br /> <br />`requestedInterval`: The interval for the attempted new segment allocation. <br /><br />`allocatedInterval` The interval allocated for the requested segment |


Nit: This fault says that the requested segment could not be allocated, but something else could be.
In that sense, it doesn't seem too different from the InsertCannotAllocateSegment.

Do you think just a different error message in the same fault type would work?

…ularity (apache#14475) Changes: - Throw an `InsertCannotAllocateSegmentFault` if the allocated segment is not aligned with the requested granularity. - Tests to verify new behaviour

initial commit

6fbbcc4

kfaraz added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Jun 23, 2023

kfaraz reviewed Jun 23, 2023

View reviewed changes

kfaraz added the Bug label Jun 23, 2023

review comments

881e8e5

LakshSingla force-pushed the msq-allocation-regress branch from 51fc129 to 881e8e5 Compare June 26, 2023 04:39

github-actions Bot added the Area - Documentation label Jun 26, 2023

spelling

1a61c14

kfaraz approved these changes Jun 26, 2023

View reviewed changes

LakshSingla added 2 commits June 26, 2023 13:47

equals fix

06f3565

delete existing error class

8bf00cf

kfaraz merged commit f546cd6 into apache:master Jun 27, 2023

abhishekagarwal87 added this to the 27.0 milestone Jul 19, 2023

LakshSingla mentioned this pull request Aug 18, 2023

Unifying MSQ's exception system and DruidExceptions #14871

Open

	public static boolean doesIntervalMatchesGranularity(final Interval interval, final Granularity granularity)
	public static boolean isAlignedWithGranularity(final Interval interval, final Granularity granularity)

Conversation

LakshSingla commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LakshSingla commented Jun 26, 2023

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LakshSingla Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LakshSingla commented Jun 23, 2023 •

edited

Loading

LakshSingla Jun 26, 2023 •

edited

Loading