Skip to content

Refactor WindowOperatorQueryKit to use WindowStage class for representing different window stages#17158

Merged
cryptoe merged 13 commits intoapache:masterfrom
Akshat-Jain:refactor-WindowOperatorQueryKit-stage-creation
Nov 12, 2024
Merged

Refactor WindowOperatorQueryKit to use WindowStage class for representing different window stages#17158
cryptoe merged 13 commits intoapache:masterfrom
Akshat-Jain:refactor-WindowOperatorQueryKit-stage-creation

Conversation

@Akshat-Jain
Copy link
Copy Markdown
Contributor

@Akshat-Jain Akshat-Jain commented Sep 25, 2024

Description

This PR refactors WindowOperatorQueryKit to use a new class WindowStage to represent the different window stages. Previously we were dealing with List<OperatorFactory> or List<List<OperatorFactory>>, and doing a bunch of instanceof checks, so this refactor makes it much more readable and cleaner.

This PR does not have any functional change.


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 Sep 25, 2024
@Akshat-Jain
Copy link
Copy Markdown
Contributor Author

@kgyrtkirk Thank you for the review!

I was wondering if it makes sense to get #17038 merged first, and then resolve conflicts on this PR, instead of the other way round? Plus this PR is much smaller than the other one, so it would allow us to iterate faster on this after getting the other PR merged.

If we try to merge this PR first, a lot of this PR's logic would have to be re-worked when resolving conflicts on the GlueingPartitioningOperator PR. That PR is already huge, it would be unnecessary to re-work this PR's logic there.

Thoughts?

@kgyrtkirk
Copy link
Copy Markdown
Member

we will merge the PR which is ready first

I've suggested to open this from that pr here - so I think the natural order is to have this one finished first

@Akshat-Jain Akshat-Jain requested a review from kgyrtkirk October 12, 2024 11:40
@cryptoe cryptoe merged commit c571e69 into apache:master Nov 12, 2024
jtuglu1 pushed a commit to jtuglu1/druid that referenced this pull request Nov 20, 2024
@adarshsanjeev adarshsanjeev added this to the 32.0.0 milestone Jan 16, 2025
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.

4 participants