Skip to content

Conversation

davidjb
Copy link
Contributor

@davidjb davidjb commented Jan 15, 2021

The current syntax for CREATE INDEX's filter_predicate appears to imply that only one or two conditions (joined with AND) are supported:

[snip]

    [ WHERE <filter_predicate> ]

[snip]

<filter_predicate> ::=
    <conjunct> [ AND <conjunct> ]

<conjunct> ::=
    <disjunct> | <comparison>

[snip]

In other words, one conjunct is required and a second may optionally be provided. However, in testing with SQL Server 2019 (spun up using Docker), an arbitrary number of conjuncts are allowed, with a query such as this succeeding and creating the index:

CREATE UNIQUE INDEX [constraint] ON [test_table] ([_type])
WHERE
  ([status] = 'in_progress'
  AND [status] IN ('needs_changes', 'published')
  AND [status] = 'foo'
  AND [status] = 'bar'
  AND [status] = 'baz'
  AND [status] IN ('more', 'disjuncts'));

Given the above syntax works, a [...n] convention needs to be added to
indicate multiple conditions are acceptable
(https://docs.microsoft.com/en-us/sql/t-sql/language-elements/transact-sql-syntax-conventions-transact-sql?view=sql-server-ver15). This PR follows https://docs.microsoft.com/en-us/sql/powershell/query-expressions-and-uniform-resource-names?view=sql-server-ver15#syntax - the one example in this repo's BNF syntax.

Let me know if further changes are necessary.

@PRMerger12
Copy link
Contributor

@davidjb : Thanks for your contribution! The author(s) have been notified to review your proposed change.

@ktoliver ktoliver added the aq-pr-triaged tracking label for the PR review team label Jan 15, 2021
Copy link
Contributor

@pmasl pmasl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should remain as .
BTW, the same approach is used in other similar examples, including the AND keyword page, without it being confused: https://docs.microsoft.com/en-us/sql/t-sql/language-elements/and-transact-sql?view=sql-server-ver15

Why is it especially confusing in this one?

@PRMerger13
Copy link
Contributor

@davidjb : Thanks for your contribution! The author(s) have been notified to review your proposed change.

@davidjb davidjb changed the title Recursive syntax for CREATE INDEX filter_predicate Add repeat to CREATE INDEX filter_predicate syntax Jan 18, 2021
@davidjb
Copy link
Contributor Author

davidjb commented Jan 18, 2021

@pmasl The current documentation doesn't indicate that that the AND condition can be repeated. I've just changed my PR to use the repeat syntax as set out in this repo's conventions.

So, the BNF used for CREATE INDEX currently states that either 1 or 2 conditions are all that are allowed (in other words, no repeat syntax):

<filter_predicate> ::=
    <conjunct> [ AND <conjunct> ]

By comparison, a page like https://docs.microsoft.com/en-us/sql/powershell/query-expressions-and-uniform-resource-names?view=sql-server-ver15#syntax shows multiple conditions are acceptable:

<FilterExpression>::=  
<PropertyExpression> [and <PropertyExpression>][...n]  

My updated PR uses this latter formatting (albeit with whitespace like the other repeat syntax in this file) compared to what I had previously.

As for the documentation page for AND, that covers syntax of exactly two Boolean expressions, whereas this relates to an indeterminate limit, given that my example shows that you can have as many conditions as you'd like for this type of query. A closer example is the Search Condition syntax, where an indeterminate number of AND/OR conditions can be specified:

<search_condition_without_match> ::= 
    { [ NOT ] <predicate> | ( <search_condition_without_match> ) }   
    [ { AND | OR } [ NOT ] { <predicate> | ( <search_condition_without_match> ) } ]   
[ ...n ]  

If there are other pages where a [...n] isn't present, adding it to there too would help avoid confusion for future readers. 👍

@PRMerger16
Copy link
Contributor

@davidjb : Thanks for your contribution! The author(s) have been notified to review your proposed change.

@PRMerger14
Copy link
Contributor

@davidjb : Thanks for your contribution! The author(s) have been notified to review your proposed change.

@github-actions github-actions bot added the stale label Mar 4, 2021
@davidjb
Copy link
Contributor Author

davidjb commented Mar 4, 2021

#sign-off , but I’m not sure the bot’s comments are aimed at me as author vs reviewer.

@PRMerger9
Copy link
Contributor

@davidjb: I'm sorry - only the author of this article, @pmasl, can sign off on your changes. But we do have an exception process - if you are on the Microsoft content or product team for this product area, you can ask the PR review team to review and merge it by sending mail to the techdocprs alias.

@rothja rothja added the keep-open Do not automatically close due to inactivity. label Mar 4, 2021
@MicrosoftDocs MicrosoftDocs deleted a comment from github-actions bot Mar 4, 2021
@github-actions github-actions bot removed the stale label Mar 5, 2021
@ktoliver
Copy link
Contributor

ktoliver commented Mar 5, 2021

#sign-off , but I’m not sure the bot’s comments are aimed at me as author vs reviewer.

@davidjb It's for the reviewer. Thanks.

@davidjb
Copy link
Contributor Author

davidjb commented Mar 6, 2021

#sign-off , but I’m not sure the bot’s comments are aimed at me as author vs reviewer.

@davidjb It's for the reviewer. Thanks.

Thanks, I understand that now @ktoliver after seeing the bot’s reply. The bot’s original comment was ambiguous (“when you are finished”) — perhaps the bot could only comment when a PR is fully approved or @ mention the reviewer (since it knows who they are in the reply) or just mention it’s “for reviewers only”. Any of those would resolve the ambiguity.

Copy link
Contributor

@pmasl pmasl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this change @ktoliver please merge

The [current syntax](https://docs.microsoft.com/en-us/sql/t-sql/statements/create-index-transact-sql?view=sql-server-ver15#syntax-for-sql-server-and-azure-sql-database) for CREATE INDEX's `filter_predicate` appears to imply that only one or two conditions (joined with `AND`) are supported:

```
[snip]
    [ WHERE <filter_predicate> ]
[snip]
<filter_predicate> ::=
    <conjunct> [ AND <conjunct> ]

<conjunct> ::=
    <disjunct> | <comparison>

[snip]
```

In other words, one `conjunct` is required and a second may optionally be provided.  However, in testing with SQL Server 2019 (spun up using Docker), an arbitrary number of `conjunct`s are allowed, with a query such as this succeeding and creating the index:

```sql
CREATE UNIQUE INDEX [constraint] ON [test_table] ([_type])
WHERE
  ([status] = 'in_progress'
  AND [status] IN ('needs_changes', 'published')
  AND [status] = 'foo'
  AND [status] = 'bar'
  AND [status] = 'baz'
  AND [status] IN ('more', 'disjuncts'));
```

Given the above syntax works, a `[...n]` convention needs to be added to
indicate multiple conditions are acceptable
(https://docs.microsoft.com/en-us/sql/t-sql/language-elements/transact-sql-syntax-conventions-transact-sql?view=sql-server-ver15).  This PR follows https://docs.microsoft.com/en-us/sql/t-sql/queries/search-condition-transact-sql?view=sql-server-ver15 and  https://docs.microsoft.com/en-us/sql/powershell/query-expressions-and-uniform-resource-names?view=sql-server-ver15#syntax - examples in this repo's BNF syntax of multiple, optional conditions.
@PRMerger15
Copy link
Contributor

@davidjb : Thanks for your contribution! The author(s) have been notified to review your proposed change.

@PRMerger15 PRMerger15 requested a review from pmasl June 11, 2021 06:36
@davidjb
Copy link
Contributor Author

davidjb commented Jun 11, 2021

I've made the change made accordingly from @pmasl's feedback.

@pmasl
Copy link
Contributor

pmasl commented Jun 11, 2021

#sign-off

@ktoliver ktoliver merged commit 0156aea into MicrosoftDocs:live Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aq-pr-triaged tracking label for the PR review team Change sent to author keep-open Do not automatically close due to inactivity. ready-to-merge sql/prod t-sql/tech
Projects
None yet
Development

Successfully merging this pull request may close these issues.