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

Port SqlParameter changes from netcore to netfx #1812

Merged
merged 6 commits into from
Nov 2, 2022

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Oct 24, 2022

Ports the changes in SqlParameters perf optimizations to netfx #209.
This is a prerequisite for #19

@JRahnama
Copy link
Member

@Wraith2 Thanks for the PR. The test pipelines are setup, but there are some adjustments left to be done. I saw some failures on net462 and not sure if you were able to see them.
image

@JRahnama JRahnama added the ➕ Code Health Changes related to source code improvements label Oct 24, 2022
@JRahnama JRahnama added this to In Progress in SqlClient v5.1 via automation Oct 24, 2022
@JRahnama JRahnama added this to the 5.1.0-preview2 milestone Oct 24, 2022
@Wraith2
Copy link
Contributor Author

Wraith2 commented Oct 24, 2022

It turns out you have to actually set the parameters field so they can be sent to the server or all sorts of things stop working.

The CI doesn't seem happy because legs i haven't change (enclave ones) are failing for net6.0 builds. I'm also not sure which ones i should be paying attention to. Do we still have legacy checks?

@JRahnama
Copy link
Member

It turns out you have to actually set the parameters field so they can be sent to the server or all sorts of things stop working.

The CI doesn't seem happy because legs i haven't change (enclave ones) are failing for net6.0 builds. I'm also not sure which ones i should be paying attention to. Do we still have legacy checks?

I will take the old one out today. The one I was working on, this will be primary one today, is CI-SqlClient-To-Be-Main.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Oct 25, 2022

Most legs are passing but some fail with EXEC(0)

@JRahnama
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov
Copy link

codecov bot commented Oct 26, 2022

Codecov Report

Base: 71.38% // Head: 71.39% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (27a369c) compared to base (20d4c19).
Patch coverage: 96.23% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1812      +/-   ##
==========================================
+ Coverage   71.38%   71.39%   +0.01%     
==========================================
  Files         290      290              
  Lines       61236    61265      +29     
==========================================
+ Hits        43712    43741      +29     
  Misses      17524    17524              
Flag Coverage Δ
addons 92.38% <ø> (ø)
netcore 74.97% <100.00%> (-0.04%) ⬇️
netfx 69.32% <96.04%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...Microsoft/Data/SqlClient/TdsParserHelperClasses.cs 63.85% <88.88%> (+0.89%) ⬆️
...t/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs 73.16% <96.27%> (+0.59%) ⬆️
...netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs 83.81% <100.00%> (+0.04%) ⬆️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 71.05% <100.00%> (+<0.01%) ⬆️
.../src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs 87.89% <0.00%> (-4.49%) ⬇️
...core/src/Microsoft/Data/SqlClient/SqlDataReader.cs 72.46% <0.00%> (-0.33%) ⬇️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 81.35% <0.00%> (-0.23%) ⬇️
...lient/src/Microsoft/Data/SqlClient/SqlParameter.cs 75.39% <0.00%> (-0.11%) ⬇️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 73.52% <0.00%> (-0.03%) ⬇️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Oct 31, 2022

Can you kick the CI again please.

@DavoudEshtehari
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Wraith2
Copy link
Contributor Author

Wraith2 commented Oct 31, 2022

The failures are odd but appear repeatable. They're in Always Encrypted tests which i have no way to debug locally because of the sql server requirements. I'm going to need some help from the team to investigate why those failures are occuring.

As i mentioned in the first post this is a blocking issue for the sql batch api, without these changes there is no way to access the parameter collection for the set of rpc's in the batch.

@DavoudEshtehari
Copy link
Member

The failures don't require Always Encrypted enabled.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Nov 1, 2022

Ok, all green now. quick merge it before I break it again!

SqlClient v5.1 automation moved this from In Progress to Review Approved Nov 2, 2022
@JRahnama JRahnama merged commit fe403ff into dotnet:main Nov 2, 2022
SqlClient v5.1 automation moved this from Review Approved to Done Nov 2, 2022
SqlClient v5.1 automation moved this from Done to Review Approved Nov 2, 2022
@Wraith2 Wraith2 deleted the port-sqlparameter-perf branch November 2, 2022 01:03
@DavoudEshtehari DavoudEshtehari moved this from Review Approved to Done in SqlClient v5.1 Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
➕ Code Health Changes related to source code improvements
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants