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.

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

Allow execution of highlighted subquery #3288

Merged
merged 9 commits into from Jan 20, 2019

Conversation

Projects
None yet
3 participants
@chang
Copy link
Contributor

commented Jan 16, 2019

This PR allows the user to execute just the highlighted part of an adhoc query, which often comes in handy when iterating on subqueries.

query_section

@ghost ghost added the in progress label Jan 16, 2019

@chang chang changed the title Feature: Allow execution of highlighted subquery Allow execution of highlighted subquery Jan 16, 2019

@arikfr
Copy link
Member

left a comment

Thank you for taking at a stab at this! There are currently two issues with your implementation:

  1. If you save the query while there is highlighted text, it will save the partial query. This is not what the user expects though. The selection should only affect the execution. Maybe when executing the query, we will "ask" the editor for the current query value or just have a way to update the query screen with the current "active query text" (which can be the highlighted text)?

  2. When we receive a result, we update the last query result reference to point at this result. If this happens to be a partial result, and the user later saves the query it will reference this result. We need to change the logic to only update the reference if this was an execution of the whole query. I can add a pointer to where this happens in code.

@chang

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

Thanks for the quick review @arikfr! I'd be happy to iterate on this.

I can add a pointer to where this happens in code.

That would be great.

@arikfr

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

Great, I appreciate the effort!

We update the reference here:

$scope.query.latest_query_data_id = $scope.queryResult.getId();

You can compare the query text of the query result object with the one of the query, and update if needed. This will mean that parameterized queries will no longer store this reference, but this was wrong anyway.

As for executing the correct query text, this is the handler for the Execute button:

$scope.executeQuery = () => {

Which in turn calls this getQueryResult:

function getQueryResult(maxAge) {
if (maxAge === undefined) {
maxAge = $location.search().maxAge;
}
if (maxAge === undefined) {
maxAge = -1;
}
$scope.showLog = false;
$scope.queryResult = $scope.query.getQueryResult(maxAge);
}

Which eventually calls the Query service getQueryResult method. It currently takes only the maxAge parameter, but for this feature we can add an optional query parameter, which will override the current query text.

@ranbena

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

@arikfr Perhaps missing an indication that current execution is from selection and not full query. Not a must. Thinking of current users who'd be unaware the editor functionality changed.

@ranbena
Copy link
Member

left a comment

🐞 The "Save" button shows "dirty*" indication upon selection, which is wrong, as the query content remains unchanged (and saving has no affect).

@arikfr

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

@ranbena

Perhaps missing an indication that current execution is from selection and not full query. Not a must. Thinking of current users who'd be unaware the editor functionality changed.

Good point. It makes sense to change the button text to "Execute Highlighted" when there is a selection.

The "Save" button shows "dirty*" indication upon selection, which is wrong, as the query content remains unchanged (and saving has no affect).

This is because we update the Query's query value... Indeed not an expected behavior.

@ranbena

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

This is because we update the Query's query value... Indeed not an expected behavior.

Another bug it introduces - when saving query while text selected, the selected text gets saved as the full query.

@ranbena

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

It makes sense to change the button text to "Execute Highlighted" when there is a selection.

I think it could prove too "jumpy" and distracting.
Another approach is an indication on execution instead of on selection.
For instance, change selection color, making it obvious that the selection is the one getting executed.

webp net-gifmaker

@arikfr

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

@ranbena,

I think it could prove too "jumpy" and distracting.
Another approach is an indication on execution instead of on selection.
For instance, change selection color, making it obvious that the selection is the one getting executed.

Maybe this + change the tooltip on the Execute button?

@chang

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

I've refactored to maintain state of the highlighted text in view.js and stop modifying queryText. This is so the mod+enter shortcut can also get the highlighted text:

const shortcuts = {
'mod+enter': $scope.executeQuery,
};

That should fix the save-related bugs. Couple questions about the UI:

@ranbena How do you change the text highlight color as it's executing? I couldn't figure this out.
@arikfr What do you mean by "change the tooltip"? The tooltip says CTRL+ENTER right now, so I don't think you meant to change that to "Execute Selected".

Currently I have the text change to "Execute Selected" but we can get rid of that and just have the highlight color change.

Also, the CI failure seems to be unrelated. I'm a javascript novice so thanks for the help, and any suggestions for refactoring are welcome.
selected

Show resolved Hide resolved client/app/services/query.js Outdated
@arikfr

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

Currently I have the text change to "Execute Selected" but we can get rid of that and just have the highlight color change.

It's actually good this way 👍

chang added some commits Jan 18, 2019

@chang

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

SGTM - here's where we are, but I'm sure there's a better way to implement the fade.
kapture 2019-01-18 at 2 24 35

@ranbena
Copy link
Member

left a comment

Works well apart for a regression due to the Ace classname manipulation.
Here's a fix you can merge in chang#1

chang added some commits Jan 18, 2019

Merge pull request #1 from getredash/select-exec-bg
Fix query selection execution background color
@chang

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

Thanks for the fix! Merged and I think this is ready.

@arikfr

arikfr approved these changes Jan 20, 2019

@arikfr arikfr merged commit 8bc8e2d into getredash:master Jan 20, 2019

9 of 12 checks passed

stickler-ci Review failed
Details
Header rules No header rules processed
Details
Pages changed 7 new files uploaded
Details
Mixed content No mixed content detected
Details
Redirect rules 7 redirect rules processed
Details
WIP Ready for review
Details
build Workflow: build
Details
codeclimate All good!
Details
deploy/netlify Deploy preview ready!
Details
percy/redash Visual review automatically approved, no visual changes found.
Details
security/snyk - package.json (arikfr) No new issues
Details
security/snyk - requirements.txt (arikfr) No manifest changes detected

@ghost ghost removed the in progress label Jan 20, 2019

@arikfr

This comment has been minimized.

Copy link
Member

commented Jan 20, 2019

👏

Well done, @chang. Thank you for this great feature and the quick follow ups. 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.