-
Notifications
You must be signed in to change notification settings - Fork 473
New docs on expressions and constants #1008
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
Conversation
ba26800
to
fe1b2e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoa! Amazing work, @knz! Thank you so much for taking this on.
I'm reviewing in two batches. Here are suggestions/questions on the changes to the existing data type pages. Next, I'll review the new pages.
bytes.md
Outdated
- String literal: `'a1b2c3'` | ||
|
||
You can also use these in combination, for example, `b'\141\061\x62\x32\c3'`. | ||
When it is not ambiguous, a string literal can also be automatically |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this statement ambiguous ;). @arjunravinarayan also wasn't quite certain about the intention. Can you be more explicit about when a string is not ambiguous? What that means exactly? Perhaps an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reworded to clarify.
data-types.md
Outdated
- `<value>::<data type>`, or its equivalent longer form `CAST(<value> AS <data type>)`, which converts an arbitrary expression of one built-in type to another (this is also known as type coercion or "casting"). For example: | ||
`NOW()::DECIMAL`, `VARIANCE(a+2)::INT`. | ||
|
||
**Note that in many cases it is advisable to use an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tip! 2 suggestions:
- Instead of bolding the whole thing, let's format it as an actual tip. Here's how (plus some suggested copy edits):
{{site.data.alerts.callout_success}}
In many cases, it is recommended to use a [type annotation](sql-expressions.html#explicitly-typed-expression) instead of a coercion to create a value of an arbitrary type.
{{site.data.alerts.end}}
- Should we add a bit of explanation about why type annotation is better in most cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added the callout thing and a more clearly worded recommendation.
date.md
Outdated
The string format for dates is `YYYY-MM-DD`. For example: `DATE '2016-12-23'`. | ||
|
||
Note that in some contexts, dates may be displayed with hours, minutes, seconds, and timezone set to 0. | ||
When it is unambiguous, a simple unannotated string literal can also |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as my comment on bytes.md
. I don't understand what we're trying to say here. Can you please be more explicit, rephrase, perhaps include an example, if necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
float.md
Outdated
|
||
## Format | ||
|
||
When inserting into a `FLOAT` column, format the value as a numeric literal, e.g., `1.2345` or `1`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change the section title to Syntax
and replace the first sentence with:
Values of type `FLOAT` must be expressed as [numeric literals](sql-constants.html#numeric-literals).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
For more details about `FLOAT` numeric formats, | ||
see the section on [SQL constants](sql-constants.html). | ||
|
||
The special IEEE754 values for positive infinity, negative infinity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change interpreted literal
to a link:
[interpreted literal](sql-constants.html#interpreted-literals)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
int.md
Outdated
### Hexadecimal-Encoded Numeric Literal | ||
|
||
When inserting a hexadecimal-encoded numeric literal into a `INT` column, format the value as hexadecimal digits preceded by `0x`. For example, `0xcafe1111` corresponds to the numeric literal `3405648145`. | ||
See the section on [numeric literals](sql-constants.html#numeric-literals) for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, let's change the section title to Syntax
and replace this sentence with:
Values of type `INT` must be expressed as [numeric literals](sql-constants.html#numeric-literals).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
--- | ||
|
||
The `SERIAL` [data type](data-types.html) defaults to a unique 64-bit signed integer. The default value is the combination of the insert timestamp and the ID of the node executing the insert. This combination is guaranteed to be globally unique. Also, because value generation does not require talking to other nodes, it is much faster than sequentially auto-incrementing a value, which requires distributed coordination. | ||
The `SERIAL` [data type](data-types.html) is a column data type which |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for strengthening this! While you're here, can you rename the Format
section to Syntax
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
string.md
Outdated
## Formats | ||
|
||
A `STRING` column accepts Unicode string literals, hexadecimal string literals, and escape strings. | ||
A `STRING` constant can be expressed using a variety of formats, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's retitle this section Syntax
and, for consistency, rephrase the content a bit:
Values of type `STRING` can be expressed in a variety of formats. See [string literals](sql-constants.html#string-literals) for more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
string.md
Outdated
For example, the `e'x61\141\u0061'` escape string represents the hexadecimal byte, octal byte, and 16-bit hexadecimal Unicode character values equivalent to the `'aaa'` string literal. | ||
|
||
Note that any character not in the table above is taken literally in an escape string. Also, when continuing an escape string across lines, write `e` or `E` only before the first opening quote. | ||
When printing out a `STRING` value in the SQL shell, the shell uses the simple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make SQL shell a link:
[SQL shell](use-the-built-in-sql-client.html)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
[coerced to](sql-expressions.html#explicit-type-coercions) type | ||
`TIMESTAMP`/`TIMESTAMPTZ`. | ||
|
||
`TIMESTAMP` constants can be expressed using the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You removed this type of guidance from other type pages - I assume because the details are available in your value expressions doc? Should we do the same here, or is there a risk that they won't find the correct details via link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DATE/INTERVAL/TIMESTAMP constants are not lexical elements and thus not distinguishable as separate constant categories in the page "SQL constants". However I agree that a mutual xref was missing; I added it.
Review status: 0 of 14 files reviewed at latest revision, 17 unresolved discussions, some commit checks failed. bytes.md, line 20 at r1 (raw file):
Quick note before I do a more thorough review: for this and all other data types, I would like to see at least one example of formatting left on the page. The idea being that I can jump to the page and get the information I'm most likely to need, and then find a link to deeper reference material only if I need it. Comments from Reviewable |
bytes.md, line 20 at r1 (raw file): Previously, sploiselle (Sean Loiselle) wrote…
@knz, although I didn't say as much in my review, I agree with Sean that simple examples on page are a huge plus for getting users quick answers. I'd say a simple example per expression type mentioned, here and other data type pages. Comments from Reviewable |
Quick note before I do a more thorough review: for this and all
other data types, I would like to see at least one example of
formatting left on the page. The idea being that I can jump to the
page and get the information I'm most likely to need, and then find
a link to deeper reference material only if I need it.
@knz <https://github.com/knz>, although I didn't say as much in my
review, I agree with Sean that simple examples on page are a huge plus
for getting users quick answers. I'd say a simple example per expression
type mentioned, here and other data type pages.
I agree with both of you on this. I'll have a look before Monday.
Cheers
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, amazing work, @knz. Thanks very much for writing this up!
I have lots of little comments and suggestions, some content-driven, some format-driven. I'm happy to take such revisions on in a follow-up PR, if you prefer. Just let me know.
Also, we should think about how to represent these pages in the sidenav. Not sure what the best approach is here. Might want to create a new section under Develop
for these pages as well as, for example, the keywords and identifiers page?
sql-expressions.md
Outdated
[table expressions](table-expressions.html) which produce results | ||
structured as a table. | ||
|
||
A value expression can be: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's necessary to provide this list, given that the auto-generated toc provides the same exact list. Instead, I'd suggest we:
- Replace the single intro sentence ("Value expressions allow...") with the first two paragraphs of the
Introduction
section. - And then delete the entire
Introduction
section.
After the two intro paragraphs, and before the toc, we could have a sentence like: The following sections provide details on possible types of value expressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
|
||
## Introduction | ||
|
||
Most SQL statements can contain *value expressions* that compute new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comma after SELECT ceil(price) FROM items
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
items` the expression `ceil(price)` computes the rounded-up value of | ||
the values from the `price` column. | ||
|
||
Value expressions produce values suitable to store in a single table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comma after the table expressions
link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
sql-expressions.md
Outdated
Constant expressions represent a simple value that doesn't change. | ||
They are described further in section [SQL Constants](sql-constants.html). | ||
|
||
## Column references |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this and all other section headings, let's use title case, i.e., Column References
instead of Column references
.
sql-expressions.md
Outdated
|
||
## Column references | ||
|
||
An expression can refer to columns in the current query in two ways: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be clearer as: An expression in a query can refer to columns in two ways:
sql-constants.md
Outdated
[+-]9999.[9999][e[+-]999] | ||
[+-][9999].9999[e[+-]999] | ||
[+-]9999e[+-]999 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
sql-constants.md
Outdated
``` | ||
The actual data type of a numeric constant depends both on the context | ||
where it is used, its literal format and its numeric value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use comma before and its numeric value
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
sql-constants.md
Outdated
## Byte array literals | ||
A byte array literal uses the same syntax as string literals containing character escapes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make [string literals](#string-literals)
a link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
sql-constants.md
Outdated
The two differences between byte array literals and string literals with character escapes are: | ||
- byte array literals always have data type `BYTES`, whereas the data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalize first letter of each bullet item, and end each with a period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait what? The paragraph starts with "The two differences ..." above and ends with the last bullet point.
sql-constants.md
Outdated
CockroachDB recognizes the following SQL named constants: | ||
- `TRUE` and `FALSE`, the two possible values of data type `BOOL`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change ;
to .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
ebca42c
to
6640c43
Compare
I have reworked the sidebar significantly. PTAL |
sql-constants.md, line 85 at r1 (raw file): Previously, knz (kena) wrote…
Parentheses are still there ...? Comments from Reviewable |
sql-constants.md, line 174 at r1 (raw file): Previously, knz (kena) wrote…
Yes, you're right. I should have also asked you to rework the intro to the list to be a complete sentence, something like:
Comments from Reviewable |
sql-expressions.md, line 60 at r1 (raw file): Previously, knz (kena) wrote…
Whoa! Thank you. Comments from Reviewable |
sql-expressions.md, line 69 at r1 (raw file): Previously, knz (kena) wrote…
You're misunderstanding what I meant. I envision adding manually created content to the page with auto-generated content, not getting the auto-generator to produce this type of content. In any case, we can revisit later. This content is great here as well. Comments from Reviewable |
sql-expressions.md, line 198 at r1 (raw file): Previously, knz (kena) wrote…
Instead of using a list to form a complete sentence, we introduce a list with a complete sentence and then treat each bullet as an independent item. Can each of these bullets stand on its own, or are they dependent on each other? If the former, please don't use semicolons. Comments from Reviewable |
sql-expressions.md, line 264 at r1 (raw file): Previously, knz (kena) wrote…
OK, point take, and we'll revisit that. But if a column can't contain an array, then we should say "For example, if column Comments from Reviewable |
sql-expressions.md, line 365 at r1 (raw file): Previously, knz (kena) wrote…
OK, thanks. Comments from Reviewable |
sql-expressions.md, line 419 at r1 (raw file): Previously, knz (kena) wrote…
Looks like this didn't actually get done? Comments from Reviewable |
sql-statements.md, line 2 at r2 (raw file):
This is a huge improvement. Thank you, @knz! Comments from Reviewable |
table-expressions.md, line 7 at r1 (raw file): Previously, knz (kena) wrote…
OK. Comments from Reviewable |
table-expressions.md, line 156 at r1 (raw file): Previously, knz (kena) wrote…
Sorry to have misdirected you here. Can you reformat the the callout to include only html formatting, not markdown? {{site.data.alerts.callout_info}} Comments from Reviewable |
Thanks for making so many tweaks, @knz. I've included/repeated a few more, but it's look very close now. I would like to decouple this work from an overahaul of the sidenav, however. There are some strange user experience issues that need to be factored in when designing the sidenav (page url needs to match link url in sidenav in order for the correct section to expand and the sidnav item to get highlighted, for example). And we're already intending to rethink the docs architecture (mostly this section) next quarter, as well as move to another tool that will let us restructure our site hierarchy. So for now, I'd suggest we go back to the old sidenav with the three new pages simply added to the Comments from Reviewable |
Reviewed 22 of 22 files at r2. Comments from Reviewable |
I'm very confused by the sidenav syntax and I can't seem to get it to work now (although it worked yesterday!). Are you ok with adding this fix after merging this PR? Review status: 1 of 16 files reviewed at latest revision, 18 unresolved discussions, some commit checks pending. data-types.md, line 31 at r1 (raw file): Previously, jseldess wrote…
Done. date.md, line 47 at r2 (raw file): Previously, jseldess wrote…
Done. manual-deployment.md, line 12 at r2 (raw file): Previously, jseldess wrote…
Done. sql-constants.md, line 2 at r1 (raw file): Previously, jseldess wrote…
Done. sql-constants.md, line 30 at r1 (raw file): Previously, jseldess wrote…
Done. sql-constants.md, line 85 at r1 (raw file): Previously, jseldess wrote…
I had forgotten to commit the change. Apologies. sql-constants.md, line 174 at r1 (raw file): Previously, jseldess wrote…
Done. sql-expressions.md, line 69 at r1 (raw file): Previously, jseldess wrote…
Ok I agree! sql-expressions.md, line 198 at r1 (raw file): Previously, jseldess wrote…
Done. sql-expressions.md, line 264 at r1 (raw file): Previously, jseldess wrote…
Ok now I see where you're coming from. We were writing past each other by overlapping the notion of "physical column in storage" and "virtual column of an intermediate result table in a query". I reworked the explanation accordingly. sql-expressions.md, line 419 at r1 (raw file): Previously, jseldess wrote…
Again forgot to commit. sql-statements.md, line 2 at r2 (raw file): Previously, jseldess wrote…
😸 table-expressions.md, line 156 at r1 (raw file): Previously, jseldess wrote…
Done. Comments from Reviewable |
Just extended this PR to integrate the changes from cockroachdb/cockroach#13094 |
Reviewed 16 of 21 files at r3. Comments from Reviewable |
* Add note about non-root users * Update getting-started.md * Update getting-started.md * Update oasis/getting-started.md Co-authored-by: Simran <Simran-B@users.noreply.github.com> * Update getting-started.md Co-authored-by: ansoboleva <93702078+ansoboleva@users.noreply.github.com> Co-authored-by: Simran <Simran-B@users.noreply.github.com>
Fixes #992.
This change is