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

Tell a bit more about SQL name resolution. #1050

Merged
merged 1 commit into from Feb 1, 2017

Conversation

knz
Copy link
Contributor

@knz knz commented Jan 31, 2017

This change is Reviewable

@sploiselle
Copy link
Contributor

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion.


sql-name-resolution.md, line 7 at r1 (raw file):

---

Table and function names can exist in multiple places. Resolution decides which one to use.

Such a welcome surprise to get these PRs from you, @knz!

So I can get a better understanding of what this page does for users, can I get you to reframe this introductory paragraph? I like to think of the first line of a technical document as "making sure the reader knows what they get by reading this doc." I think this is most of the way there, but I'm wondering if we can make this a little more explicit? When/why would someone use this page? What are the actions someone has recently taken that would lead them to need the info you've outlined here?


Comments from Reviewable

@knz
Copy link
Contributor Author

knz commented Jan 31, 2017

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion.


sql-name-resolution.md, line 7 at r1 (raw file):

Previously, sploiselle (Sean Loiselle) wrote…

Such a welcome surprise to get these PRs from you, @knz!

So I can get a better understanding of what this page does for users, can I get you to reframe this introductory paragraph? I like to think of the first line of a technical document as "making sure the reader knows what they get by reading this doc." I think this is most of the way there, but I'm wondering if we can make this a little more explicit? When/why would someone use this page? What are the actions someone has recently taken that would lead them to need the info you've outlined here?

I just moved some text at the beginning for this purpose. PTAL


Comments from Reviewable

@jseldess
Copy link
Contributor

jseldess commented Feb 1, 2017

LGTM, with some comments. Thanks for this addition, @knz!

In a separate PR, I've arrange general SQL syntax topics under a new subsection of Develop. I'll put this there once it's been merged.


Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions.


sql-expressions.md, line 36 at r2 (raw file):

  configuration`.

  In case the column name is ambiguous (e.g. when joining across

I'd reformat this section a bit:

  • Using the name of the column, e.g., price in SELECT price FROM items.
    • If the name of the column is a SQL keyword, the name just be quoted, e.g., SELECT "Default" FROM configuration.
    • If the name is ambiguous (e.g., when joining across multiple tables), it can be disambiguated by prefixing the column name with the table name, e.g., SELECT items.price FROM items.
  • Using the ordinal position of the column, e.g., SELECT items.price FROM items.

sql-expressions.md, line 283 at r2 (raw file):
This sentence is a bit confusing. Let's combine with the previous sentence and clarify, something like:

This applies the named function to the arguments between parentheses. When the function's name space is not included, name resolution rules determine which function is called.


sql-name-resolution.md, line 7 at r1 (raw file):
This is a great addition, @knz. Thank you.

General American English style nit: use a comma after e.g. and i.e.. Feel free to adjust here and in other files (didn't catch this earlier), or I can take care of it later.

In second sentence, for readability, let's move the example into parentheses:

The same table name (e.g., orders) can exist in multiple databases.

In third sentence, let's stick with e.g. instead of for example. This is something we should look at more globally, but not important now. And let's move it into parentheses:

When a query specifies a table name without a database name (e.g., SELECT * FROM orders;), how does CockroachDB know which orders table is being considered?


sql-name-resolution.md, line 24 at r2 (raw file):

function names in [value expressions](sql-expressions.html):

- If the given name already tells where to look explicitly, i.e. it is *qualified*, then just use this information.

Let's make the two top-level bullets a bit more direct:

  • If the name is qualified (i.e., the name already tells where to look), use this information.
  • If the name is unqualified:

sql-name-resolution.md, line 28 at r2 (raw file):

  - Try to find the name in the "default database" as set by [`SET DATABASE`](set-database.html).
  - Try to find the name using the [search path](#search-path).
   - If the name was not found so far, produce an error.

nit: If the name is not found, produce an error.


sql-name-resolution.md, line 39 at r2 (raw file):

The current search path can be inspected using the statement `SHOW
SEARCH_PATH`, or `SHOW ALL`.

Make SHOW ALL a link:

[`SHOW ALL`](show-all.html)

sql-name-resolution.md, line 44 at r2 (raw file):
Maybe clarify the opening of this paragraph a bit:

By default, the search path for new sessions is pg_catalog, ...

Also a nit: Remove quotes around pg_catalog.


Comments from Reviewable

@knz
Copy link
Contributor Author

knz commented Feb 1, 2017

Review status: all files reviewed at latest revision, 7 unresolved discussions.


sql-expressions.md, line 36 at r2 (raw file):

Previously, jseldess wrote…

I'd reformat this section a bit:

  • Using the name of the column, e.g., price in SELECT price FROM items.
    • If the name of the column is a SQL keyword, the name just be quoted, e.g., SELECT "Default" FROM configuration.
    • If the name is ambiguous (e.g., when joining across multiple tables), it can be disambiguated by prefixing the column name with the table name, e.g., SELECT items.price FROM items.
  • Using the ordinal position of the column, e.g., SELECT items.price FROM items.

Done.


sql-expressions.md, line 283 at r2 (raw file):

Previously, jseldess wrote…

This sentence is a bit confusing. Let's combine with the previous sentence and clarify, something like:

This applies the named function to the arguments between parentheses. When the function's name space is not included, name resolution rules determine which function is called.

Done.


sql-name-resolution.md, line 7 at r1 (raw file):

Previously, jseldess wrote…

This is a great addition, @knz. Thank you.

General American English style nit: use a comma after e.g. and i.e.. Feel free to adjust here and in other files (didn't catch this earlier), or I can take care of it later.

In second sentence, for readability, let's move the example into parentheses:

The same table name (e.g., orders) can exist in multiple databases.

In third sentence, let's stick with e.g. instead of for example. This is something we should look at more globally, but not important now. And let's move it into parentheses:

When a query specifies a table name without a database name (e.g., SELECT * FROM orders;), how does CockroachDB know which orders table is being considered?

I have adjusted these occurrences but also some others I found in sql-expressions.md. Thanks for pointing this out.


sql-name-resolution.md, line 24 at r2 (raw file):

Previously, jseldess wrote…

Let's make the two top-level bullets a bit more direct:

  • If the name is qualified (i.e., the name already tells where to look), use this information.
  • If the name is unqualified:

Done.


sql-name-resolution.md, line 28 at r2 (raw file):

Previously, jseldess wrote…

nit: If the name is not found, produce an error.

Done.


sql-name-resolution.md, line 39 at r2 (raw file):

Previously, jseldess wrote…

Make SHOW ALL a link:

[`SHOW ALL`](show-all.html)

Done.


sql-name-resolution.md, line 44 at r2 (raw file):

Previously, jseldess wrote…

Maybe clarify the opening of this paragraph a bit:

By default, the search path for new sessions is pg_catalog, ...

Also a nit: Remove quotes around pg_catalog.

The quotes around the second occurrence is because we're talking about the text of the prefix, not the thing named pg_catalog.


Comments from Reviewable

@jseldess
Copy link
Contributor

jseldess commented Feb 1, 2017

:lgtm:


Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@jseldess
Copy link
Contributor

jseldess commented Feb 1, 2017

There's a broken internal link preventing the tests from succeeding, but I'll fix that in a follow-up PR.

@jseldess jseldess merged commit e2b4e31 into cockroachdb:gh-pages Feb 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants