Added isExecuteImmediate selector to resource groups
#27079
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…urce group DB update for table `selectors`: `is_execute_immediate VARCHAR(6)`
Reviewer's GuideThis PR introduces a new boolean ER diagram for selectors table with is_execute_immediate columnerDiagram
SELECTORS {
BIGINT resource_group_id
INT priority
VARCHAR user_regex
VARCHAR source_regex
VARCHAR original_user_regex
VARCHAR authenticated_user_regex
VARCHAR query_type
VARCHAR client_tags
VARCHAR selector_resource_estimate
VARCHAR user_group_regex
VARCHAR is_execute_immediate
}
RESOURCE_GROUPS {
BIGINT resource_group_id
VARCHAR environment
}
SELECTORS ||--o{ RESOURCE_GROUPS : "resource_group_id"
Class diagram for updated PreparedQuery and SelectionCriteriaclassDiagram
class QueryPreparer {
+prepareQuery(Session, Statement): PreparedQuery
}
class PreparedQuery {
-Statement statement
-List<Expression> parameters
-Optional<String> prepareSql
-boolean isExecuteImmediate
+getStatement()
+getParameters()
+getPrepareSql()
+isExecuteImmediate()
}
class DispatchManager {
-createQueryInternal(...)
}
class SelectionCriteria {
-boolean authenticated
-String user
-Optional<String> source
-Set<String> clientTags
-ResourceEstimates resourceEstimates
-Optional<String> queryType
-boolean isExecuteImmediate
+isExecuteImmediate()
}
QueryPreparer --> PreparedQuery
DispatchManager --> SelectionCriteria
PreparedQuery --> SelectionCriteria: isExecuteImmediate
Class diagram for SelectorSpec, SelectorRecord, and StaticSelector with isExecuteImmediateclassDiagram
class SelectorSpec {
-Optional<Boolean> isExecuteImmediate
+isExecuteImmediate()
}
class SelectorRecord {
-Optional<Boolean> isExecuteImmediate
+isExecuteImmediate()
}
class StaticSelector {
-Optional<Boolean> isExecuteImmediate
}
SelectorSpec <|-- SelectorRecord
SelectorSpec <|-- StaticSelector
StaticSelector --> SelectorSpec: isExecuteImmediate
SelectorRecord --> SelectorSpec: isExecuteImmediate
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `plugin/trino-resource-group-managers/src/main/java/io/trino/plugin/resourcegroups/db/ResourceGroupsDao.java:84` </location>
<code_context>
" query_type VARCHAR(512),\n" +
" client_tags VARCHAR(512),\n" +
" selector_resource_estimate VARCHAR(1024),\n" +
+ " is_execute_immediate VARCHAR(6),\n" +
" FOREIGN KEY (resource_group_id) REFERENCES resource_groups (resource_group_id)\n" +
")")
</code_context>
<issue_to_address>
**suggestion:** Consider using a more appropriate type for is_execute_immediate column.
Using VARCHAR for boolean values can cause confusion and compromise data integrity. Prefer BOOLEAN or a constrained CHAR(1) if available for better validation.
Suggested implementation:
```java
" is_execute_immediate BOOLEAN,\n" +
```
If there are any places in your codebase where you insert or query the `is_execute_immediate` column, ensure that you use boolean values (`true`/`false`) instead of strings. You may also need to update any related model classes or SQL queries to expect a boolean type.
</issue_to_address>
### Comment 2
<location> `plugin/trino-resource-group-managers/src/test/java/io/trino/plugin/resourcegroups/TestStaticSelector.java:224-225` </location>
<code_context>
assertThat(selector.match(newSelectionCriteria("A.user", "a source b", ImmutableSet.of("tag1", "tag2", "tag3"), EMPTY_RESOURCE_ESTIMATES)).map(SelectionContext::getResourceGroupId)).isEqualTo(Optional.of(resourceGroupId));
}
+ @Test
+ public void testIsExecuteImmediate()
+ {
+ ResourceGroupId resourceGroupId = new ResourceGroupId(new ResourceGroupId("global"), "foo");
</code_context>
<issue_to_address>
**suggestion (testing):** Missing negative and edge case tests for isExecuteImmediate selector.
Please add tests for cases where isExecuteImmediate is false, unset, or does not match, to fully validate selector behavior.
</issue_to_address>
### Comment 3
<location> `plugin/trino-resource-group-managers/src/test/java/io/trino/plugin/resourcegroups/db/TestDbSourceExactMatchSelector.java:56` </location>
<code_context>
+ assertThat(selector.match(new SelectionCriteria(true, "testuser", ImmutableSet.of(), "testuser", Optional.empty(), Optional.of("@test@test_pipeline"), ImmutableSet.of("tag"), EMPTY_RESOURCE_ESTIMATES, Optional.empty(), false))).isEqualTo(Optional.empty());
</code_context>
<issue_to_address>
**suggestion (testing):** No tests for isExecuteImmediate=true in DbSourceExactMatchSelector.
Add tests covering cases where isExecuteImmediate is true to ensure correct selector behavior.
</issue_to_address>
### Comment 4
<location> `plugin/trino-resource-group-managers/src/test/java/io/trino/plugin/resourcegroups/TestResourceGroupIdTemplate.java:51-52` </location>
<code_context>
+ StaticSelector selector = new StaticSelector(Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty(), Optional.of(sourcePattern), Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty(), template);
</code_context>
<issue_to_address>
**suggestion (testing):** No test coverage for isExecuteImmediate in ResourceGroupIdTemplate tests.
Please add tests for isExecuteImmediate in ResourceGroupIdTemplate, focusing on template variable extraction cases.
</issue_to_address>
### Comment 5
<location> `docs/src/main/sphinx/admin/resource-groups.md:209-212` </location>
<code_context>
client-provided tags associated with the query.
+- `isExecuteImmediate` (optional): Some clients use `EXECUTE IMMEDIATE ...`
+ to run quick queries directly. Use `true` when this selector should match a
+ `EXECUTE IMMEDIATE '...'` query. Use `false` to match all other
+ queries, omit to match any.
+
</code_context>
<issue_to_address>
**suggestion (typo):** Change 'a EXECUTE IMMEDIATE' to 'an EXECUTE IMMEDIATE' for grammatical correctness.
Use 'an EXECUTE IMMEDIATE ... query' for correct grammar.
```suggestion
- `isExecuteImmediate` (optional): Some clients use `EXECUTE IMMEDIATE ...`
to run quick queries directly. Use `true` when this selector should match an
`EXECUTE IMMEDIATE ...` query. Use `false` to match all other
queries, omit to match any.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...source-group-managers/src/main/java/io/trino/plugin/resourcegroups/db/ResourceGroupsDao.java
Show resolved
Hide resolved
Comment on lines
+51
to
+52
| StaticSelector selector = new StaticSelector(Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty(), Optional.of(sourcePattern), Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty(), template); | ||
| SelectionCriteria context = new SelectionCriteria(true, "user", ImmutableSet.of(), "user", Optional.empty(), Optional.of("scheduler.important.testpipeline[5]"), ImmutableSet.of(), EMPTY_RESOURCE_ESTIMATES, Optional.empty(), false); |
There was a problem hiding this comment.
suggestion (testing): No test coverage for isExecuteImmediate in ResourceGroupIdTemplate tests.
Please add tests for isExecuteImmediate in ResourceGroupIdTemplate, focusing on template variable extraction cases.
Comment on lines
+209
to
+212
| - `isExecuteImmediate` (optional): Some clients use `EXECUTE IMMEDIATE ...` | ||
| to run quick queries directly. Use `true` when this selector should match a | ||
| `EXECUTE IMMEDIATE '...'` query. Use `false` to match all other | ||
| queries, omit to match any. |
There was a problem hiding this comment.
suggestion (typo): Change 'a EXECUTE IMMEDIATE' to 'an EXECUTE IMMEDIATE' for grammatical correctness.
Use 'an EXECUTE IMMEDIATE ... query' for correct grammar.
Suggested change
| - `isExecuteImmediate` (optional): Some clients use `EXECUTE IMMEDIATE ...` | |
| to run quick queries directly. Use `true` when this selector should match a | |
| `EXECUTE IMMEDIATE '...'` query. Use `false` to match all other | |
| queries, omit to match any. | |
| - `isExecuteImmediate` (optional): Some clients use `EXECUTE IMMEDIATE ...` | |
| to run quick queries directly. Use `true` when this selector should match an | |
| `EXECUTE IMMEDIATE ...` query. Use `false` to match all other | |
| queries, omit to match any. |
gertjanal
changed the title
isExecuteImmediate selector to resource groups selectorsisExecuteImmediate selector to resource groups
|
I feel that this may be understanding the use case of cc our language expert @martint in case I am misunderstanding anything about the intended semantics |
|
I concur with @xkrogen above. Starburst has implemented this feature I believe - https://docs.starburst.io/latest/admin/resource-groups.html#selector-rules
|
|
@xkrogen you are correct. There is no reason to treat it differently from any other query. That said, this change isn't something we want to accept and merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
External clients might use the
EXECUTE IMMEDIATE '..'query wrapper. This type of query is used for small and quick queries, that should be answered within seconds (to select partition info for a table for example). When using resource groups, theseEXECUTE IMMEDIATEqueries are scheduled as normal queries, as theQueryPreparerunwraps the statement and forgets that is was aEXECUTE IMMEDIATEquery. This results in queued small queries and unresponsive user interfaces.With this PR, a selector can be created to match all
EXECUTE IMMEDIATEqueries by setting"isExecuteImmediate": true. The selector can send the query to a priority resource group so that the query is handled directly.Additional context and related issues
I added the
isExecuteImmediateto thePreparedQuery, as theQueryPreparerunwraps theExecuteImmediateStatementand forgets it's purpose. I considered keeping thePreparedQueryand adding aboolean isExecuteImmediate = query.toLowerCase().trim().startsWith("EXECUTE IMMEDIATE ");to theDispatchManager, but I don't think the query should be "parsed" twice.Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( X ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:
Summary by Sourcery
Add support for matching EXECUTE IMMEDIATE queries in resource group selectors by introducing an isExecuteImmediate flag and propagating it through query preparation, dispatch, configuration, and storage.
New Features:
Enhancements:
Build:
Documentation:
Tests: