Skip to content

Add msqDenySelect planner config to (dis)allow MSQ SELECT queries.#13992

Closed
abhishekrb19 wants to merge 3 commits intoapache:masterfrom
abhishekrb19:ingest_only_querycontext_taskapi
Closed

Add msqDenySelect planner config to (dis)allow MSQ SELECT queries.#13992
abhishekrb19 wants to merge 3 commits intoapache:masterfrom
abhishekrb19:ingest_only_querycontext_taskapi

Conversation

@abhishekrb19
Copy link
Contributor

@abhishekrb19 abhishekrb19 commented Mar 28, 2023

Description

This PR adds a new planner configuration msqDenySelect that, when set to true, MSQ SELECT statements will be blocked from planning. This parameter is set to false by default, so all queries will plan.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.

@abhishekrb19 abhishekrb19 force-pushed the ingest_only_querycontext_taskapi branch from 7a948ac to d4f0db8 Compare March 28, 2023 21:01
@abhishekrb19
Copy link
Contributor Author

It looks like only 1 GHA check ran. The tests didn't seem to run

@abhishekrb19 abhishekrb19 marked this pull request as draft March 28, 2023 22:20
@abhishekrb19 abhishekrb19 changed the title Add msqDenySelect query context parameter to (dis)allow MSQ SELECT queries. Add msqDenySelect planner context config to (dis)allow MSQ SELECT queries. Mar 29, 2023
@abhishekrb19 abhishekrb19 marked this pull request as ready for review March 29, 2023 00:19
@abhishekrb19 abhishekrb19 changed the title Add msqDenySelect planner context config to (dis)allow MSQ SELECT queries. Add msqDenySelect planner config to (dis)allow MSQ SELECT queries. Mar 29, 2023
@Test
public void testMsqDenySelectEnabledQuery()
{
msqCompatible();
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests probably want to be in CalciteInsertDmlTest, if they can share config. Those tests are run only for MSQ. The test in this form runs for both MSQ and non-MSQ, which may not be what you wanted.

}

@Test
public void testMsqDenySelectDisabledQuery()
Copy link
Contributor

Choose a reason for hiding this comment

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

A test that denies access should probably expect an error, I would think.

public static final boolean DEFAULT_ENABLE_DEBUG = false;
public static final int DEFAULT_IN_SUB_QUERY_THRESHOLD = Integer.MAX_VALUE;
public static final boolean DEFAULT_ENABLE_TIME_BOUNDARY_PLANNING = false;
public static final boolean DEFAULT_MSQ_DENY_SELECT = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

No longer needed.

@FrankChen021
Copy link
Member

It looks like only 1 GHA check ran. The tests didn't seem to run

This's because the ASF recently requires committers to click a 'apporve' button to run CI. We discussed this matter in dev mailing thread and wanted to change it back.

@FrankChen021 FrankChen021 added the Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 label Mar 29, 2023
@abhishekrb19
Copy link
Contributor Author

Closing this in favor of a different approach which would include query kind in the EXPLAIN PLAN output.

@abhishekrb19 abhishekrb19 deleted the ingest_only_querycontext_taskapi branch April 10, 2023 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

3 participants