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

Improve accuracy for Table Value Constructor #2115

Merged
merged 2 commits into from May 23, 2019
Merged

Conversation

srutzky
Copy link
Contributor

@srutzky srutzky commented May 10, 2019

This PR resolves issue #2090 .

  1. Reword main description such that it does not imply that the USING clause of the MERGE statement is a third and separate case from INSERT ... VALUES and derived tables. It is a derived table as the USING clause is really just a FROM clause (even the MERGE documentation states as much by directing the reader to view the documentation for the FROM clause: https://docs.microsoft.com/en-us/sql/t-sql/statements/merge-transact-sql#arguments ).
  2. Changed the wording of "directly in the VALUES list of an INSERT ... VALUES statement" to be "as the VALUES clause of an INSERT ... VALUES statement":
    a. removed "directly" since it is technically a "direct" usage whether here or as a derived table
    b. use "as ... clause" instead of "in ... list" since "in" implies that the "VALUES" keyword is already there, which would end up being "VALUES (VALUES (), (), ... )" which is incorrect. Since the "VALUES" keyword is an inherent part of the Table Value Constructor (TVC), it cannot be that a TVC is used "in" a VALUES list, which assumes that VALUES is already present. Yes, nit-picky, but this phrasing is more accurate.
  3. Removed the redundant restatement of the two places where the TVC can be used. That was stated clearly in the main description at the top of the page.
  4. State the actual limit. We know it's 1000 rows, so why not state it?
  5. State clearly that the row limit does not apply when using the TVC as a derived table.
  6. Add 2 methods for bulk inserting: .NET SqlBulkCopy class, and OPENROWSET(BULK ...)
  7. For all 4 bulk insert methods, link to the actual documentation (why make the reader do an extra search for them?). Not 100% sure about the relative path for the .NET documentation. That might need to be an absolute path. Not sure how to test that prior to submitting.
  8. Added newline to TVC in Example A to prevent forcing the reader to need to scroll sideways.
  9. Added Example E to illustrate what is meant by "use a derived table to insert more than 1000 rows".

I have tested on SQL Server 2012, 2017, and 2019 CTP 2.5.

For more details, please see: https://sqlquantumleap.com/2019/05/09/maximum-number-of-rows-for-the-table-value-constructor/

1. Reword main description such that it does not imply that the `USING` clause of the `MERGE` statement is a third and separate case from `INSERT ... VALUES` and derived tables. It is a derived table as the `USING` clause is really just a `FROM` clause (even the `MERGE` documentation states as much by directing the reader to view the documentation for the `FROM` clause: https://docs.microsoft.com/en-us/sql/t-sql/statements/merge-transact-sql#arguments ).
2. Changed the wording of "directly _in_ the VALUES list of an INSERT ... VALUES statement" to be "_as_ the VALUES clause of an INSERT ... VALUES statement":
     a. removed "directly" since it is technically a "direct" usage whether here or as a derived table
     b. use "as ... clause" instead of "in ... list" since "in" implies that the "VALUES" keyword is already there, which would end up being "VALUES (VALUES (), (), ... )" which is incorrect. Since the "VALUES" keyword is an inherent part of the Table Value Constructor (TVC), it cannot be that a TVC is used "in" a VALUES list, which assumes that VALUES is already present. Yes, nit-picky, but this phrasing is more accurate.
3. Removed the redundant restatement of the two places where the TVC can be used. That was stated clearly in the main description at the top of the page.
4. State the actual limit. We know it's 1000 rows, so why not state it?
5. State clearly that the row limit does _not_ apply when using the TVC as a derived table.
6. Add 2 methods for bulk inserting: .NET SqlBulkCopy class, and OPENROWSET(BULK ...)
7. For all 4 bulk insert methods, link to the actual documentation (why make the reader do an extra search for them?). Not 100% sure about the relative path for the .NET documentation. That might need to be an absolute path. Not sure how to test that prior to submitting.
8. Added newline to TVC in Example A to prevent forcing the reader to need to scroll sideways.
9. Added Example E to illustrate what is meant by "use a derived table to insert more than 1000 rows".

I have tested on SQL Server 2012, 2017, and 2019 CTP 2.5.

For more details, please see: https://sqlquantumleap.com/2019/05/09/maximum-number-of-rows-for-the-table-value-constructor/
@PRMerger10
Copy link
Collaborator

@PRMerger10 PRMerger10 commented May 10, 2019

@srutzky : Thanks for your contribution! The author, @VanMSFT, has been notified to review your proposed change.

@PRMerger18
Copy link
Collaborator

@PRMerger18 PRMerger18 commented May 23, 2019

@VanMSFT : Thanks for your contribution! The author, @VanMSFT, has been notified to review your proposed change.

@VanMSFT
Copy link
Member

@VanMSFT VanMSFT commented May 23, 2019

@srutzky - Hey Solomon! Thanks for the edits! Again, it's very much appreciated. I made some minor changes, but please feel free to let me know if there are any issues. I'll go ahead and sign-off for now, and check the links once they are live to make any changes again.

#sign-off

@srutzky
Copy link
Contributor Author

@srutzky srutzky commented May 23, 2019

Looks good.

Would be super-awesome if there was a way to get an accurate preview that resolved includes, relative links, etc. 😸

@VanMSFT
Copy link
Member

@VanMSFT VanMSFT commented May 23, 2019

Unfortunately, not in our public PRs. I agree though, it would be great to have.

@Jak-MS
Copy link
Contributor

@Jak-MS Jak-MS commented May 23, 2019

testing PRMerger labelling

#sign-off

@Jak-MS Jak-MS merged commit 24553a3 into MicrosoftDocs:live May 23, 2019
1 check passed
@MightyPen
Copy link
Contributor

@MightyPen MightyPen commented May 23, 2019

This PR 2115 caused a Docs build break in private repo 'sql-docs-pr'. The following link, near line 63, is invalid:

WRONG: [SqlBulkCopy class](../../../dotnet/api/system.data.sqlclient.sqlbulkcopy.md)

Correct link would have been:

RIGHT: [SqlBulkCopy class](/dotnet/api/system.data.sqlclient.sqlbulkcopy)

The reason is that we are limited to how high we can navigate with '../' nodes.
We cannot upward navigate the https address's directory node path above https://docs.microsoft.com/sql/.

But if we really need to, then we must switch to using the special kinda fullpath syntax (that starts with '/', as shown).
And when we do this, we must remove the trailing '.md' (just as we would have to for a literally full https address like - https://docs.microsoft.com/sql/sql-server/sql-server-technical-documentation.

Mike is fixing the failure.
Thanks.

@srutzky
Copy link
Contributor Author

@srutzky srutzky commented May 23, 2019

@MightyPen Sounds good. I will try to remember this detail for next time. :-)

@VanMSFT
Copy link
Member

@VanMSFT VanMSFT commented May 23, 2019

@MightyPen @MikeRayMSFT - Thanks for fixing this, guys!

@MightyPen
Copy link
Contributor

@MightyPen MightyPen commented May 23, 2019

@srutzky We cannot expect members of the public to know these link nuances. But if you are ever unsure about one part of a large many-line edit that you submit, it can be helpful if mention the line to us in the text of the PR.
If just one line is being changed, then our eyes will focus on the line anyway. :-)
Thanks again. We DO appreciate your contributions, which we find to be of above average value.

@srutzky
Copy link
Contributor Author

@srutzky srutzky commented May 26, 2019

@MightyPen Sorry about breaking the build, I didn't even know that could happen. Yes, I will be sure to highlight if I am unsure of a link to something (or something along those lines). To be fair, I did at least try to mention it (item 7 in the original description), but apparently wasn't successful 😿 . From now on, I will be sure to boldify it, and even separate it if necessary.

@srutzky srutzky deleted the patch-6 branch Jul 9, 2019
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

6 participants