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

Add execution options to the stream factory #848

Merged
merged 1 commit into from Oct 3, 2019
Merged

Conversation

aaomidi
Copy link
Contributor

@aaomidi aaomidi commented Aug 13, 2019

The output factory was never getting the new query options if it was made before.

Let me know if I'm making a mistake here!

@aaomidi aaomidi requested a review from kburtram August 14, 2019 17:31
@kburtram
Copy link
Member

@aaomidi the change looks fine conceptually to me, but I'm not sure what specific problem this is solving. Are there cases where the query execution code isn't getting the user's settings?

@aaomidi
Copy link
Contributor Author

aaomidi commented Aug 20, 2019

Yeah, the sqltoolsservice doesn't register most of the settings in: https://github.com/microsoft/sqltoolsservice/blob/master/src/Microsoft.SqlTools.ServiceLayer/SqlContext/QueryExecutionSettings.cs

Except a few specific ones in random places:

https://github.com/microsoft/sqltoolsservice/blob/master/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs#L120

https://github.com/microsoft/sqltoolsservice/blob/master/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs#L524-L534

https://github.com/microsoft/sqltoolsservice/blob/master/src/Microsoft.SqlTools.ServiceLayer/QueryExecution/Query.cs#L540-L567

However, the settings that don't have a special condition written for them in the Query file don't actually get applied.

I ran into this issue with not being able to increase the XML with MaxXmlCharsToStore setting.

@kburtram kburtram merged commit e95b4c8 into master Oct 3, 2019
@kburtram kburtram deleted the execution_options branch October 3, 2019 17:45
@kburtram
Copy link
Member

kburtram commented Oct 3, 2019

@aaomidi I'll merge this now based on discussion in DRI meeting yesterday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants