topn with granularity regression fixes by clintropolis · Pull Request #17565 · apache/druid

clintropolis · 2024-12-13T02:28:23Z

Description

changes:

fix issue where topN with query granularity other than ALL would use the heap algorithm when it was actual able to use the pooled algorithm, and incorrectly used the pool algorithm in cases where it must use the heap algorithm, a regression from rework cursor creation #16533
fix issue where topN with query granularity other than ALL could incorrectly process values in the wrong time bucket, another regression from rework cursor creation #16533

This PR has:

been self-reviewed.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
been tested in a test Druid cluster.

changes: * fix issue where topN with query granularity other than ALL would use the heap algorithm when it was actual able to use the pooled algorithm, and incorrectly used the pool algorithm in cases where it must use the heap algorithm, a regression from apache#16533 * fix issue where topN with query granularity other than ALL could incorrectly process values in the wrong time bucket, another regression from apache#16533

kgyrtkirk · 2024-12-13T07:38:57Z

      final int numValuesPerPass = numBytesPerRecord > 0 ? numBytesToWorkWith / numBytesPerRecord : cardinality;

-      return numValuesPerPass <= cardinality;
+      return numValuesPerPass >= cardinality;


this comparision change caught my eye...the new cmp seem to be more in line with the intention - but it seems to me that
depending on some conditions here ; this cardinality's value might be -1 in some cases
is that ok to answer true if cardinality is -1 ?

note: if cardinality == -1 ; then both the old and the new logic returns true - is that ok?

yea this is fair, it is not really a problem in practice because currently the column capabilities should not report as dictionary encoded if the cardinality is -1, so we return false earlier in the method before we get here, but this seems worth explicitly checking for just in case.

…ty-stuff

gianm · 2024-12-13T21:19:49Z

  )
  {
+    if (cardinality < 0) {
+      // unknown cardinality doesn't work with the pooled algorith which requires an exact count of dictionary ids


algorithm (spelling)

gianm · 2024-12-13T21:24:54Z

        return true;
      } else {
-        if (!cursor.isDone() && granularizer.currentOffsetWithinBucket()) {
+        if (granularizer.currentOffsetWithinBucket()) {


was the cursor.isDone() here removed simply because it's redundant?

yea, i noticed granularizer.currentOffsetWithinBucket also checks isDone so it seemed nicer this way, this didn't cause any issues, just noticed while i was looking over stuff

gianm · 2024-12-13T21:39:17Z

-          aggregator.init(resultsBuffer, position);
-          aggregator.aggregate(resultsBuffer, position);
-          positionToAllocate += aggregatorSize;
+    if (granularizer.currentOffsetWithinBucket()) {


was the granularizer.currentOffsetWithinBucket() check added here (& the other prototypes) to enable skipping of empty granular buckets?

yea, this was a bug, i did it as an if statement wrapping the loop so we wouldn't need to check currentOffsetWithinBucket() twice per loop since the granularizer advance methods also call currentOffsetWithinBucket

…when multi-passes is required even wihen query granularity is not all

gianm · 2024-12-16T23:54:30Z

            "context": {
-                "useCache": "true",
-                "populateCache": "true",
+                "useCache": "false",


why change this?

was trying to see if could allow the tests to pass without setting the new context parameter, but it didn't seem to help, so changed to use the new context parameter.

gianm · 2024-12-16T23:58:35Z

  public static final String UNCOVERED_INTERVALS_LIMIT_KEY = "uncoveredIntervalsLimit";
  public static final String MIN_TOP_N_THRESHOLD = "minTopNThreshold";
  public static final String CATALOG_VALIDATION_ENABLED = "catalogValidationEnabled";
+  public static final String TOPN_USE_MULTI_PASS_POOLED_QUERY_GRANULARITY = "topNuseMultiPassPooledQueryGranularity";


Please add a javadoc comment here explaining what this is for, and linking to this PR (or a related issue). It's helpful to have that kind of thing for any undocumented parameter.

cryptoe · 2024-12-17T15:51:13Z

Since @gianm comments were non blocking, going ahead with merge since this blocks 31.0.1 release.

cryptoe · 2024-12-17T17:11:03Z


  private CursorGranularizer(
      Cursor cursor,
+      Granularity granularity,


@clintropolis Do we need this field ?

* topn with granularity regression fixes changes: * fix issue where topN with query granularity other than ALL would use the heap algorithm when it was actual able to use the pooled algorithm, and incorrectly used the pool algorithm in cases where it must use the heap algorithm, a regression from apache#16533 * fix issue where topN with query granularity other than ALL could incorrectly process values in the wrong time bucket, another regression from apache#16533 * move defensive check outside of loop * more test * extra layer of safety * move check outside of loop * fix spelling * add query context parameter to allow using pooled algorithm for topN when multi-passes is required even wihen query granularity is not all * add comment, revert IT context changes and add new context flag

* topn with granularity regression fixes (#17565) * topn with granularity regression fixes changes: * fix issue where topN with query granularity other than ALL would use the heap algorithm when it was actual able to use the pooled algorithm, and incorrectly used the pool algorithm in cases where it must use the heap algorithm, a regression from #16533 * fix issue where topN with query granularity other than ALL could incorrectly process values in the wrong time bucket, another regression from #16533 * move defensive check outside of loop * more test * extra layer of safety * move check outside of loop * fix spelling * add query context parameter to allow using pooled algorithm for topN when multi-passes is required even wihen query granularity is not all * add comment, revert IT context changes and add new context flag * remove unused

clintropolis added the Bug label Dec 13, 2024

clintropolis added this to the 31.0.1 milestone Dec 13, 2024

clintropolis added 2 commits December 12, 2024 18:37

move defensive check outside of loop

76d1c6c

more test

2f61031

kgyrtkirk reviewed Dec 13, 2024

View reviewed changes

clintropolis added 4 commits December 13, 2024 02:50

extra layer of safety

3d986f1

Merge remote-tracking branch 'upstream/master' into fix-top-granulari…

129f381

…ty-stuff

move check outside of loop

63c4376

Merge remote-tracking branch 'upstream/master' into fix-top-granulari…

f8f07dd

…ty-stuff

gianm reviewed Dec 13, 2024

View reviewed changes

fix spelling

d4fcf84

gianm approved these changes Dec 13, 2024

View reviewed changes

add query context parameter to allow using pooled algorithm for topN …

9ba9679

…when multi-passes is required even wihen query granularity is not all

gianm reviewed Dec 16, 2024

View reviewed changes

add comment, revert IT context changes and add new context flag

9728e6a

cryptoe merged commit de9da37 into apache:master Dec 17, 2024

cryptoe reviewed Dec 17, 2024

View reviewed changes

clintropolis deleted the fix-top-granularity-stuff branch December 17, 2024 19:38

clintropolis mentioned this pull request Dec 17, 2024

[Backport] topn with granularity regression fixes #17580

Merged

clintropolis mentioned this pull request Dec 19, 2024

[DRAFT] 31.0.1 Release Notes #17534

Closed

gianm mentioned this pull request Apr 3, 2025

Inaccurate aggregation results when using doubleSum aggregator when doing a topN query for versions 31.0.0 and 31.0.1 #17813

Closed

Conversation

clintropolis commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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!

clintropolis Dec 13, 2024 • 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!

clintropolis Dec 13, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cryptoe commented Dec 17, 2024

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!

4 participants

clintropolis commented Dec 13, 2024 •

edited

Loading

clintropolis Dec 13, 2024 •

edited

Loading

clintropolis Dec 13, 2024 •

edited

Loading