Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add query context parameter to control limiting select rows #14476

Merged
merged 5 commits into from
Jun 28, 2023
Merged

Add query context parameter to control limiting select rows #14476

merged 5 commits into from
Jun 28, 2023

Conversation

adarshsanjeev
Copy link
Contributor

This PR adds a new query context parameter for MSQ to decide if the result of an MSQ select query needs to be limited. This will help maintain backward compatibility and help people not be surprised by truncated results.

This parameter has been documented in MultiStageQueryContext

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.

@kfaraz kfaraz added Area - Querying Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Jun 27, 2023
LakshSingla
LakshSingla reviewed Jun 27, 2023
View reviewed changes
Copy link
Contributor

@LakshSingla LakshSingla left a comment

Choose a reason for hiding this comment

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

Taken another pass at the PR. Thanks for making the change. Left a few line-level comments, the overall PR LGTM. Will approve once these get addressed

...core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQSelectDestination.java Outdated Show resolved Hide resolved
...core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQSelectDestination.java Outdated Show resolved Hide resolved
...core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQSelectDestination.java Outdated Show resolved Hide resolved
...s-core/multi-stage-query/src/main/java/org/apache/druid/msq/util/MultiStageQueryContext.java
@@ -204,6 +211,16 @@ public static int getRowsPerSegment(final QueryContext queryContext)
);
}

public static MSQSelectDestination getSelectDestination(final QueryContext queryContext)
{
return MSQSelectDestination.valueOf(
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if this isn't a valid destination? If this errors out with a cryptic error message, we should present a user-friendly message and list out the possible options for the destination. (Presence of DURABLE_STORAGE/S3 as an option should only be if durable storage is configured)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would currently return an IAE with "value not found in enum", but the error message could be improved here. I think that's a good point, that we should make it more user friendly. This is true for all other enum query context parameters (workerAssignementStrategy, clusterStatisticsMergeMode etc). I'll make this change in a separate PR and include all other query context parameters as well.

One thought, maybe DURABLE_STORAGE should be present always so that the user knows that it is an option and can configure durable storage if needed. If it is not enabled, we would throw an exception accordingly from wherever it is writing the results anyway. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, makes sense

cryptoe
cryptoe approved these changes Jun 28, 2023
View reviewed changes
Copy link
Contributor

@cryptoe cryptoe left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

@cryptoe cryptoe merged commit 233233c into apache:master Jun 28, 2023
@cryptoe
Copy link
Contributor

cryptoe commented Jun 28, 2023

Thanks @adarshsanjeev for the contribution.

@abhishekagarwal87 abhishekagarwal87 added this to the 27.0 milestone Jul 19, 2023
sergioferragut pushed a commit to sergioferragut/druid that referenced this pull request Jul 21, 2023
@adarshsanjeev @sergioferragut
Add query context parameter to control limiting select rows (apache#1…
6606265 
…4476)

* Add query context parameter to control limiting select rows

* Add unit tests

* Address review comments

* Address review comments

* Address review comments
@AmatyaAvadhanula AmatyaAvadhanula mentioned this pull request Aug 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cryptoe cryptoe cryptoe approved these changes

@LakshSingla LakshSingla LakshSingla approved these changes

Assignees
No one assigned
Labels
Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 Area - Querying
Projects
None yet
Milestone
27.0
Development

Successfully merging this pull request may close these issues.
5 participants
@adarshsanjeev @cryptoe @LakshSingla @abhishekagarwal87 @kfaraz