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

[RFC] Typing v2 #6189

Merged

Conversation

nvanbenschoten
Copy link
Member

@nvanbenschoten nvanbenschoten commented Apr 20, 2016

After beginning to implement the previous proposal, a few of the typing
rules were discovered to be undesirable. This proposal introduces a
third typing system called Summer (which has already been mostly implemented).
The new system is an extension of Morty, with the primary difference being
the notion of a "desired type" which is propagated downwards while type
checking. In practice, this eliminates the need for an expensive and arguably
excessive reliance on exact arithmetic throughout the evaluation of statements
and removes the need for a reliance on unclear and difficult to reason about implicit
type coercions.

[ci skip]


This change is Reviewable

@JackKrupansky
Copy link

One criteria I would suggest is how easy or difficult it will be to explain to a MySQL or Postgres user how they will have to change their queries to migrate to CockroachDB.

Not to imply that there can't be any differences, but at least to highlight a comprehensive set of test cases and show that they are either fully compatible or give comprehensible diagnostic messages that make it easy enough to convert from MySQL or Postgres.

Granted, all of the rules and nuances will need to be fully documented - in user-speak - but the longer the doc is, the less likely that an average user will read it. If the new system automatically does the right thing 98% of the time and gives a clear message the other 2% of the time that is easy to follow to eliminate the error without reading the fine print in the doc, you'll be set. The debate can be about whether 98% should be 99.5% or if 95% (19 out of 20 queries) is good enough.

@JackKrupansky
Copy link

One of the old issues is decimal vs. floating point, so I have to ask whether decimal constants will be implemented as a precursor to the new typing scheme.

Although supporting decimal constants does not eliminate float/decimal conversion issues since parameters will have their type inferred as decimal or float or integer from say an INSERT and then possibly used in a comparison to the other type - for example used in a comparison to a constant that lexically may type as float or integer because the length permits it. It depends on what rules the lexical analysis uses to determine whether an integer might be large enough to be a decimal or whether a number with a decimal point is always parsed as a decimal or only if it is out of range of a float value.

@nvanbenschoten
Copy link
Member Author

@JackKrupansky Untyped constants are a major part of this proposal. I encourage you to read about the constant folding phase of type checking system. The short answer is that constants (numeric and string) will be folded as far as possible before being typed, and will then be given a type based on their possible representations and their surrounding context.

@RaduBerinde
Copy link
Member

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


docs/RFCS/typing.md, line 365 [r1] (raw file):
hard to understand this sentence, grammar seems off


docs/RFCS/typing.md, line 368 [r1] (raw file):
same here


Comments from Reviewable

@nvanbenschoten
Copy link
Member Author

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


docs/RFCS/typing.md, line 365 [r1] (raw file):
Done.


docs/RFCS/typing.md, line 368 [r1] (raw file):
Better?


Comments from Reviewable

@petermattis
Copy link
Collaborator

:lgtm:

This is hugely detailed, though a bit overwhelming. The details here are almost certainly going to rot versus the implementation. And when you provide too much detail it can obscure the higher-level design. There is a balance between too much and too little information in an RFC and I think this one went a bit too far. This is a minor critique, though, and certainly nothing that should be changed now. A lot of the detail here will flow nicely into a blog post.


Review status: 0 of 1 files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.


docs/RFCS/typing.md, line 322 [r2] (raw file):
s/more desirable/more desirable results/g


docs/RFCS/typing.md, line 323 [r2] (raw file):
s/while all the while/while/g


docs/RFCS/typing.md, line 328 [r2] (raw file):
s/recursion/recursion./g


docs/RFCS/typing.md, line 331 [r2] (raw file):
s/resolution/resolution./g


docs/RFCS/typing.md, line 351 [r2] (raw file):
I'm not clear: are you proposing an extension to the SQL syntax we support, or just the syntax used in this document?


docs/RFCS/typing.md, line 393 [r2] (raw file):
The switch from resolvable to resolved in the two names is confusing. Shouldn't the latter be resolvable as well?


docs/RFCS/typing.md, line 450 [r2] (raw file):
Have you found an example where this is useful?


docs/RFCS/typing.md, line 495 [r2] (raw file):
s/with an/with a/g


docs/RFCS/typing.md, line 525 [r2] (raw file):
s/candidate/candidates/g


docs/RFCS/typing.md, line 567 [r2] (raw file):
What is this supposed to return given that max() takes 1 argument, usually a column name? I think you need a different example function.


docs/RFCS/typing.md, line 645 [r2] (raw file):
The use of the term demanded is new here. You've used required elsewhere. Is this different?


docs/RFCS/typing.md, line 683 [r2] (raw file):
s/"1"/"$1"/g?


Comments from Reviewable

@nvanbenschoten
Copy link
Member Author

@petermattis I agree that we may have put a little too much detail into this RFC amendment. That said, I think the reason for its focus on detail was pretty well warranted. The implementation of this new typing system (Summer) was an arguably significant deviation from the system previously described in this RFC. Because of this, it seemed important that we take a step back from the code and look to both prove its correctness and justify the changes we made to the old system. I think these changes succeed in both regards. Plus like you mentioned, this can all be pulled into a blog post.


Review status: 0 of 1 files reviewed at latest revision, 13 unresolved discussions, some commit checks pending.


Comments from Reviewable

@nvanbenschoten
Copy link
Member Author

Review status: 0 of 1 files reviewed at latest revision, 12 unresolved discussions, some commit checks pending.


docs/RFCS/typing.md, line 322 [r2] (raw file):
Done.


docs/RFCS/typing.md, line 323 [r2] (raw file):
Done.


docs/RFCS/typing.md, line 328 [r2] (raw file):
Done.


docs/RFCS/typing.md, line 331 [r2] (raw file):
Done.


docs/RFCS/typing.md, line 351 [r2] (raw file):
Here we are proposing a new SQL extension syntax. It is not needed for the correctness of the typing system so it wont be included in the initial implementation. Still, it will be a nice tool for developers to use to gain a greater level of confidence in their SQL statements, if desired.


docs/RFCS/typing.md, line 393 [r2] (raw file):
Done.


docs/RFCS/typing.md, line 450 [r2] (raw file):
It's very useful in allowing for better, more helpful error reporting


docs/RFCS/typing.md, line 495 [r2] (raw file):
Done.


docs/RFCS/typing.md, line 525 [r2] (raw file):
Done.


docs/RFCS/typing.md, line 567 [r2] (raw file):
Done.


docs/RFCS/typing.md, line 645 [r2] (raw file):
Done.


docs/RFCS/typing.md, line 683 [r2] (raw file):
Done.


Comments from Reviewable

@petermattis
Copy link
Collaborator

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


docs/RFCS/typing.md, line 351 [r2] (raw file):
There is very little visual difference between :: and :. Seems fine as syntax for this RFC, but I'd be very hesitant to add that as new syntax to our SQL dialect.


Comments from Reviewable

@nvanbenschoten
Copy link
Member Author

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


docs/RFCS/typing.md, line 351 [r2] (raw file):
@knz and I chose that syntax for the RFC because we thought it seemed like a natural transition from a cast (and looked like TypeScript), but I don't think either of us are strongly attached to it and your hesitation seems warranted. Any suggestions for a new syntax which you think might work better without colliding with existing syntax?

One alternative that we briefly discussed was E [T], but we thought it might get confused with array indexing when we add array support.


Comments from Reviewable

@petermattis
Copy link
Collaborator

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


docs/RFCS/typing.md, line 351 [r2] (raw file):
Yeah, E [T] would be problematic as well. This RFC is already tackling a lot. I'd drop any suggestion to extend the SQL syntax and leave that to a separate RFC.


Comments from Reviewable

@JackKrupansky
Copy link

You took out the max example, but it probably should have been GREATEST($1,$2), as well as GREATEST/LEAST($1,$2,$3) is illustrate the added complexity of variable-length argument lists.

@knz
Copy link
Contributor

knz commented Apr 25, 2016

@JackKrupansky GREATEST/LEAST are handled by the homogeneity rule, not overload resolution.

nvanbenschoten and others added 3 commits April 28, 2016 16:45
After beginning to implement the previous proposal, a few of the typing
rules were discovered to be undesirable. This proposal introduces a
third typing system called Summer (which has already been mostly implemented).

[ci skip]
@knz
Copy link
Contributor

knz commented May 4, 2016

I think given the implementation has landed and tests suggest that this works, we should merge this PR. Thoughts?

@nvanbenschoten nvanbenschoten merged commit 37eff60 into cockroachdb:master May 4, 2016
@nvanbenschoten nvanbenschoten deleted the nvanbenschoten/summer branch May 4, 2016 15:07
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.

5 participants