Skip to content

Fix summary iterator#15658

Merged
cryptoe merged 1 commit intoapache:masterfrom
LakshSingla:summary-iterator-visual-fix
Jan 17, 2024
Merged

Fix summary iterator#15658
cryptoe merged 1 commit intoapache:masterfrom
LakshSingla:summary-iterator-visual-fix

Conversation

@LakshSingla
Copy link
Contributor

@LakshSingla LakshSingla commented Jan 10, 2024

Description

This PR fixes the summary iterator to add aggregators in the correct position. The summary iterator is used when dims are not present, therefore the new change is identical to the old one, but seems more correct while reading.


Key changed/added classes in this PR

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.

Copy link
Member

@kgyrtkirk kgyrtkirk left a comment

Choose a reason for hiding this comment

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

right now I think q.getResultRowAggregatorStart() should be 0 when it gets here in every case; however the correct index is q.getResultRowAggregatorStart() + i

because we may never know what changes will land in the future its better to fix it than leave it!

@cryptoe cryptoe merged commit fc06f2d into apache:master Jan 17, 2024
@cryptoe cryptoe added this to the Druid 29.0.0 milestone Jan 17, 2024
@LakshSingla LakshSingla deleted the summary-iterator-visual-fix branch January 30, 2024 09:31
LakshSingla added a commit to LakshSingla/druid that referenced this pull request Jan 30, 2024
This PR fixes the summary iterator to add aggregators in the correct position. The summary iterator is used when dims are not present, therefore the new change is identical to the old one, but seems more correct while reading.
abhishekagarwal87 pushed a commit that referenced this pull request Jan 30, 2024
This PR fixes the summary iterator to add aggregators in the correct position. The summary iterator is used when dims are not present, therefore the new change is identical to the old one, but seems more correct while reading.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants