RFC: a new typing system for CockroachDB SQL

[knz]

cc @andreimatei @nvanbenschoten

[ci skip]

[spencerkimball]

This is by far the most impressive RFC yet. Very well written and complete. Nice job!

My preference is definitely for Morty, with suggestions to the user to use explicit casts where there is confusion. The differences between the two – as far as usability goes – are slight, so whichever is simpler to implement and less magical is preferable.

---

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[petermattis]

I have a strong preference for Morty as well. Rick is a genius, but too mercurial.

---

Review status: 0 of 1 files reviewed at latest revision, 6 unresolved discussions.

---

docs/RFCS/typing.md, line 798 [r3] (raw file):
This case is unfortunate. In Go, an untyped constant will only automatically convert if it doesn't lose precision. Can we add a similar rule to Morty? See http://play.golang.org/p/BzUkkj43f-. The exact package may even provide tools to determine if a conversion is safe (lossless).

---

docs/RFCS/typing.md, line 816 [r3] (raw file):
s/exeute/execute/g

---

docs/RFCS/typing.md, line 945 [r3] (raw file):
s/an/and/g

---

docs/RFCS/typing.md, line 964 [r3] (raw file):
I hope this cases are rare and the iterative application of type inference can be avoided.

---

docs/RFCS/typing.md, line 987 [r3] (raw file):
Expr is an interface. Interfaces cannot have fields. Perhaps the type info can be maintained separately from the AST nodes. The go/types package maintains a map from ast.Expr to type information.

---

docs/RFCS/typing.md, line 1038 [r3] (raw file):
I think we're going to need a handful of implicit coercions. Namely from int to float and from int to decimal. Unclear whether we should allow implicit coercion from float to decimal or decimal to float. I'd lean towards no unless we see compelling evidence for needing it. The untyped constant arithmetic should help immensely in making this less burdensome.

---

Comments from the review on Reviewable.io

[JackKrupansky]

Guys,

For background reference, here's a document I had written up to compare type conversion in CockroachDB with Postgres:
https://docs.google.com/document/d/13sSbupTpSfoa0UlMw9BjzufRs2zUhqcAhwptWSpZ6CM/edit?usp=sharing

Feel free to comment in the document itself, but preferably here to maintain the continuity of the discussion.

It is not a proposal or recommendation per se, but does highlight a lot of the stuff that Postgres does and how it does it.

It is a long document (over 30 pages), but the overview section is worth reading. I'd also call attention to what Postgres calls Assignment - INSERT and UPDATE, which have a slightly different semantics than either expression operators or function arguments. Also, Postgres gets a lot of mileage out of treating all string constants as having UNKNOWN type and freely allowing coercion from UNKNOWN to any type, with a bias towards TEXT if not closer type match. This UNKNOWN to any conversion makes positional parameter substitution very easy. And don't forget the non-numeric types, BOOLEAN and TEXT, especially for Assignment.

I certainly don't want this input to derail or bog down any of your proposed work, but there might be some reference material that might be useful at least as a check.

I'm fairly confident that you guys are probably better off making a solid incremental improvement in the near-term, as with this RFC, independent of what the product should have for a full, final production-ready model of type conversion that handles all of the cases Postgres handles even if not exactly 100% compatible.

I was hesitant to jump into the discussion for this RFC out of fear that the perfect might become the enemy of the good, as they say. In any case, it's good to see progress. Don't let my input in any way slow you down on the sprint to Beta.

I won't argue too strenuously for full compatibility with Postgres, but I will argue for full migration doc for users coming from Postgres, MySQL,, etc., who may wonder why certain queries are either not allowed or giving unexpected results.

Have fun!

[bdarnell]

Morty++

---

Review status: 0 of 1 files reviewed at latest revision, 10 unresolved discussions.

---

docs/RFCS/typing.md, line 546 [r3] (raw file):
Some of these examples are not present in the "Examples that go wrong" section.

---

docs/RFCS/typing.md, line 573 [r3] (raw file):
This is the only occurrence of this [*S] notation. I assume it's "unknown char/byte string". Allowing this ambiguity seems like a potential problem (see python 2).

---

docs/RFCS/typing.md, line 841 [r3] (raw file):
Is the ::int cast required? I thought it would implicitly cast due to the column type.

---

docs/RFCS/typing.md, line 939 [r3] (raw file):
s/implemented/implement/

---

docs/RFCS/typing.md, line 964 [r3] (raw file):
Yeah. If multiple iterations turn out to be common then that makes Rick more appealing. But I think this case will be pretty rare.

---

Comments from the review on Reviewable.io

[petermattis]

@JackKupransky Allowing string constants to coerce to integers is way too magical. That Postgres (and other systems) allow this is a mistake in my opinion. I think not being compatible here is a feature. We definitely will need to document what are rules are and the goal is to make the rules saner and less surprising than other systems.

---

Review status: 0 of 1 files reviewed at latest revision, 10 unresolved discussions.

---

Comments from the review on Reviewable.io

[JackKrupansky]

I presume this should be f($1::int)?

[tamird]

Reviewed 1 of 2 files at r3.
Review status: all files reviewed at latest revision, 15 unresolved discussions.

---

docs/RFCS/typing.md, line 395 [r3] (raw file):
how is the outermost [*N] known at this stage?

---

docs/RFCS/typing.md, line 405 [r3] (raw file):
s/the optimization for functions/constant function evaluation/

---

docs/RFCS/typing.md, line 518 [r3] (raw file):
why favour float over decimal?

---

docs/RFCS/typing.md, line 652 [r3] (raw file):
s/type type/type /

---

docs/RFCS/typing.md, line 945 [r3] (raw file):
s/assymetric/asymmetric/

---

docs/RFCS/typing.md, line 987 [r3] (raw file):
Could you have a struct containing an Expr and a type field?

---

Comments from the review on Reviewable.io

[knz]

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

---

docs/RFCS/typing.md, line 395 [r3] (raw file):
You're right at this stage it is too early. The outermost [*N] does not exist yet at this point.

---

docs/RFCS/typing.md, line 518 [r3] (raw file):
This is for pg compatibility but no strong feeling either way.

---

docs/RFCS/typing.md, line 546 [r3] (raw file):
Actually this precise example is the first one mentioned at the start of that section. However thanks for highlighting this, we'll cross check the list.

---

docs/RFCS/typing.md, line 573 [r3] (raw file):
Oops this is a remnant from an earlier version of the doc. In this RFC we propose that all string literals are typed "string" unless an explicit cast follows. We should have (already have?) a literal syntax for byte arrays anyways.

---

docs/RFCS/typing.md, line 798 [r3] (raw file):
What behavior would you like to see here? An error? Warning? What would you want the user to write instead?

---

docs/RFCS/typing.md, line 841 [r3] (raw file):
It's not required indeed it would be implicitely cast. The example comes from the earlier list however. Perhaps an idea to add a note in the rfc to highlight the cast is not needed.

---

docs/RFCS/typing.md, line 964 [r3] (raw file):
I'd say, let's try it out and see then. I'd even suggest trying out without iteration at all and see whether that hurts.

---

docs/RFCS/typing.md, line 971 [r3] (raw file):
Yes thanks.

---

docs/RFCS/typing.md, line 987 [r3] (raw file):
Perhaps we should go through a first implementation prototype and extend the RFC with a more concrete sw arch description once we get a feel of what a good code structure should be.

---

docs/RFCS/typing.md, line 1038 [r3] (raw file):
Actually I'd be bold and suggest that with the one implicit cast proposed already that the other implicit coercions are not needed. Again I'd suggest trying it out and see where it hurts.

---

Comments from the review on Reviewable.io

[knz]

Review status: 0 of 1 files reviewed at latest revision, 15 unresolved discussions.

---

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

---

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

---

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

---

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

---

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

---

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

---

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

---

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

---

Comments from the review on Reviewable.io

[knz]

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

---

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

---

Comments from the review on Reviewable.io

[knz]

Refers to the following issues: #4024 #4026 #3633 #4073 #4088

[knz]

Refers to #3271 #1795

[petermattis]

Review status: 0 of 1 files reviewed at latest revision, 8 unresolved discussions.

---

docs/RFCS/typing.md, line 798 [r3] (raw file):
I would like inserting 4.5 into an INT column to give an error. If the user wants an integer they should add a cast: INSERT INTO t (int_col) VALUES(4.5::int).

---

docs/RFCS/typing.md, line 964 [r3] (raw file):
Yep, sounds good.

---

Comments from the review on Reviewable.io

[tamird]

Reviewed 1 of 1 files at r5, 1 of 1 files at r8.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

Comments from the review on Reviewable.io

[knz]

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

---

docs/RFCS/typing.md, line 798 [r3] (raw file):
ok. How do you feel about INSERT INTO t(int_col) VALUES(float_col)? Also an error?

---

Comments from the review on Reviewable.io

[andreimatei]

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

---

docs/RFCS/typing.md, line 798 [r3] (raw file):
We've gone back and forth on this before, let's resume the discussion here.

We like the implicit cast from exact in some cases:

SELECT int_col + 1 FROM t
INSERT INTO t(int_col) VALUES ($1 + 1.5)   =>  $1[exact]. We probably want this query to be accepted, at least with arguments like `0.5`.

However in other cases it's less clear it's good:
SELECT int_col + 1.5 FROM t
Here we probably don't want the 1.5 to be casted as int. Instead I think we want the returned value to be a decimal.

About (1) INSERT INTO t(int_col) VALUES (4.5) I think I'm also in favor of this being an error. It would also remove the inconsistency between this and (2) below. knz was suggesting we solve that by changing our parser to always do casts of the input args to the desired type (so you could still pass 4.5 in (2), but I for one don't like that very much.

So we if want (1) to error out, we have to accept that sometimes we don't want the implicit cast; instead, we want a cast that fails when it would lose information (we can call this a type assertion). I think we somehow have to change rule 1) above. And we have two directions in which we could go: a) prefer to cast other numeric types to exact and go with exact as much as possible or b) do type assertions on exact early.
We could do b) and say that literals are typed as exact until they come in contact with a something that's typed:

• when it's part of an operator and the other side is typed
• when it's an argument to a function call for which there's a single candidate implementation and that candidate can't take exact(and INSERT and UPDATE are like function calls).
  And at the moment a literal gets a type, we error out if the conversion would be lossy.

This still leaves open the question about what happens when literals interact with untyped placeholders:

(I) SELECT ($1 + 2) + ($1 + 2.5)
(II) INSERT INTO t(int_col) VALUES ($1 + 1.5)
(III) INSERT INTO t(int_col, float_col) VALUES ($1, $1 + 4.5)

For I and II, we'd like to infer $1[exact].
For III we probably want $1[int], but $1 + 4.5 to be a float.
So, to satisfy I and II, we could say that untyped placeholders that get in contact with exact get inferred as exact. Note that this would make it legal to pass 0.5 in II, but not 0.4.

If we don't want to refuse the addition in III saying that the types are different, then we seem to be pushed in the direction of implicitly casting other types to exact, which is the opposite of what we've been doing so far. So, we could backtrack and say that any numeric type can get implicitly casted to exact (so $1[int] gets casted to exact in $1[int] + 4.5, and the result gets type asserted to float).

If we go with implicitly casting from anything to exact, this would have some performance implications - a lot of arithmetic (think row filters) would be performed as exact. If we stop with the exact early, filters would usually be performed in a native type.

I still don't really have an opinion on what to do here.

---

docs/RFCS/typing.md, line 790 [r8] (raw file):
I think here we either need to clarify more, or possibly change the rule.
"non-placeholder arguments" is not correct. If the user gives a type hint for a placeholder ($1::int), then it should definitely be used in the resolution. If she doesn't, but we know the type by the time we try to resolve, we again could use it (and introduce yet another asymmetry in the typing algorithm). Although I'd be in favor of not doing this, and making a note here somewhere that it's a deliberate choice, to minimize surprises.

---

Comments from the review on Reviewable.io

[petermattis]

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

---

docs/RFCS/typing.md, line 798 [r3] (raw file):
@knz Yes, I think inserting anything that cannot be represented as an INT into an INT column type should error.

---

Comments from the review on Reviewable.io

[knz]

Review status: 0 of 1 files reviewed at latest revision, 9 unresolved discussions.

---

docs/RFCS/typing.md, line 798 [r3] (raw file):
I have thought about this and worked Andrei's proposal into the doc. Comments welcome.

---

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

---

Comments from the review on Reviewable.io

[andreimatei]

Review status: 0 of 1 files reviewed at latest revision, 11 unresolved discussions.

---

docs/RFCS/typing.md, line 798 [r3] (raw file):
LGTM

---

docs/RFCS/typing.md, line 789 [r9] (raw file):
So, by this rule, SELECT int_col from a where int_col < 3.5 would be turned into SELECT int_col from a where int_col < TYPEASSERT(3.5, int)[int], right?
I think that's good, but it needs an example.

---

docs/RFCS/typing.md, line 796 [r9] (raw file):
discussing offline what we want out of $1 + 4 + $1::int

---

Comments from the review on Reviewable.io

[knz]

Review status: 0 of 1 files reviewed at latest revision, 11 unresolved discussions.

---

docs/RFCS/typing.md, line 789 [r9] (raw file):
Without overloads for comparisons, yes, but i also think we should have overloads for comparisons with exact on either side and each numeric type on the other side. And then if we have an overload for < (int, exact) then we get the natural behavior without any different typing rules. I'll add a comment about that.

---

Comments from the review on Reviewable.io

[andreimatei]

Review status: 0 of 1 files reviewed at latest revision, 11 unresolved discussions.

---

docs/RFCS/typing.md, line 789 [r9] (raw file):
well but then, unless we introduce a new rule, you fail with ambiguous for int_col < $1, right?

---

Comments from the review on Reviewable.io

[knz]

Review status: 0 of 1 files reviewed at latest revision, 11 unresolved discussions.

---

docs/RFCS/typing.md, line 789 [r9] (raw file):
ow. Right :( 0k let's go for the simple solution then.

---

Comments from the review on Reviewable.io

[andreimatei]

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

---

docs/RFCS/typing.md, line 810 [r10] (raw file):
wait wait is this what we agreed? Also you just seemed to say in an email that you don't want this?
I thought yesterday we were just talking about clients sending strings for placeholder values, instead of the type the server requested. And in that case I agree we should do a type assert, if it actually happens (and I can sort of imagine why a client would do that).
But for everything? You're saying that a client might produce the sql query "INSERT INTO a(int_col) VALUES (15`)"? Really? Would anyone quote values like that? I can't imagine why...

---

docs/RFCS/typing.md, line 835 [r10] (raw file):
you mean "already typed"?

---

Comments from the review on Reviewable.io

[knz]

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

---

docs/RFCS/typing.md, line 810 [r10] (raw file):
As discussed on Slack: I made a mistake in the wording, this was only intended for string literals. I fixed that. Can you check it?

---

docs/RFCS/typing.md, line 835 [r10] (raw file):
Yeah. Also it was about the "then" clauses not "when". I made it clearer.

---

Comments from the review on Reviewable.io

[andreimatei]

Reviewed 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 13 unresolved discussions.

---

docs/RFCS/typing.md, line 790 [r8] (raw file):
I'd put a note here that this is intended for interval and date, or whatever, and not for numbers and string/byte.

---

docs/RFCS/typing.md, line 858 [r11] (raw file):
well here I think we probably want to accept string for all types? Like even for int? Because I think it's actually valid in the pgwire protocol to send a string for any placeholder?
@dt can you comment on how arguments are supposed to be sent in pgwire?

---

docs/RFCS/typing.md, line 913 [r11] (raw file):
nice

---

Comments from the review on Reviewable.io

[dt]

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

---

docs/RFCS/typing.md, line 858 [r11] (raw file):
Err, I think @mjibson or @tamird would know better than I.

---

Comments from the review on Reviewable.io

[andreimatei]

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

---

docs/RFCS/typing.md, line 858 [r11] (raw file):
OK I just talked to Tamir and Matt a bit and also looked at libpq. So the idea is that, at prepare time, the server infers types and sends them back (as oID codes). At bind time (the step that precedes exec), the client sends the arguments positionally. For each argument, it specifies a "format" (different that a type). The format can be binary or text, and specifies the encoding of that argument. Every type has a text encoding, only some also have binary encodings. The client does not send an oID back, or anything to identify the type. So the server just needs to parse whatever it got assuming the type it previously inferred.

So, I suggest we remove this rule 13, as the issue of parsing these arguments is not really a typing issue. Formally Morty just assumes that it gets whatever type it asked for. We should make a note below about all this, and we can say that whomever implements the parsing of these arguments (our pgwire implementation) uses the same code/principles as a TYPEASSERT_STRING (but this has nothing to do with the AST of our query (which ideally should have been already saved from the prepare phase)).

---

Comments from the review on Reviewable.io

[knz]

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

---

docs/RFCS/typing.md, line 790 [r8] (raw file):
There's a note about that below.

---

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

---

Comments from the review on Reviewable.io

[JackKrupansky]

It would also be good to qualify what user personas the type rules are aimed at. I mean, some people can figure out the rules no matter how complex or poorly specified they are (either by experimentation or reading the code), while others need very clear, specific, and detailed rules, others need clear rules but they must be very simple and intuitive, and the less-sophisticated users really need the type system to do the right thing in an especially intuitive manner and especially cope with users attempting really dumb things. AFAICT, the current discussion is somewhere between Morty and Rick, but with some degree of uncertainty thrown into the mix.

[JackKrupansky]

I recognize the complexity injected by positional parameters and the separation of the prepare/execute process and we certainly need the rules to be complete and clear, but I do think that we should have a subset of the rules for documentation that make the type system clear and simple for non-parameter usage as a first priority, with parameter rules layered on top of that, at least from a user tutorial perspective.

[JackKrupansky]

The typing rules INSERT and UPDATE (VALUES) should also be documented separately from general SELECT expressions. Yes, they do behave like functions, sort of, but with a single signature, so the rules should be stated more simply for this "assignment" scenario, without the extra complexity needed for typing of multi-signature functions.

[knz]

@JackKrupansky I agree with your assessment that typing should be documented separately for the people who will use our SQL dialect. However this will be a different work (towards our online docs) than this RFC, which is uniquely geared towards the devs who will implement this. I can promise however that while writing the RFC we were pretty much focused on the ease to explain what is going on to a user as well; despite the wealth of details in the RFC I am rather confident our typing doc will be rather short yet effective.

[andreimatei]

LGTM

---

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

---

Comments from the review on Reviewable.io

[knz]

Hi guys unless someone raises their hand during the weekend I propose to merge the RFC on Monday with status "accepted" and a stated preference for Morty. We'll then get down to implement this thing starting next week.

[jseldess]

@JackKrupansky: I should be starting docs on data types, type conversion, prepared statements, and other related topics within the next month. Will be great to hear your thoughts.

[JackKrupansky]

I am still a little confused/concerned about what the final rules really are - are they pure-Morty as in the RFC document itself or what fraction of the discussed changes are being adopted?

My comment about doc was less about the doc itself and more about having a solid but readable spec that really does cover all the cases that have been raised. If too many of the rules are blended together (e.g., functions and INSERT/UPDATE), it does make it more difficult to review all of the fine details.

For example, for the case of attempting to insert 4.5 into an INT column, did we decide to make that an error or silently truncate? Seems like there were other spec cases up in the air as well. Again, my concern was that the spec be specific about these cases.

I know the doc is separate, but we should be able to judge what the doc effort/result might be from the spec itself.

It seems to me that at a minimum there should be a final spec that is solely "here's what it will be", as opposed to the original proposal (which was fine for that stage) which mixed two proposals. Again, if the final proposal is 100% pure Morty from the original RFC, fine, but it would help to have some clarity on the status of the changes that were raised during discussion. As it stands, the direction as stated most recently above is "stated preference for Morty", which does make it sound as if all the other changes are definitively rejected.

[andreimatei]

The conclusion was Morty (and we should indeed reflect that in the RFC).
But note that we evolved the rules in Morty a bit, and now, for example,
inserting 4.5 into an INT col is rejected.

On Tue, Feb 16, 2016 at 1:08 PM, Jack Krupansky notifications@github.com
wrote:

I am still a little confused/concerned about what the final rules really
are - are they pure-Morty as in the RFC document itself or what fraction of
the discussed changes are being adopted?

My comment about doc was less about the doc itself and more about having a
solid but readable spec that really does cover all the cases that have been
raised. If too many of the rules are blended together (e.g., functions and
INSERT/UPDATE), it does make it more difficult to review all of the fine
details.

For example, for the case of attempting to insert 4.5 into an INT column,
did we decide to make that an error or silently truncate? Seems like there
were other spec cases up in the air as well. Again, my concern was that the
spec be specific about these cases.

I know the doc is separate, but we should be able to judge what the doc
effort/result might be from the spec itself.

It seems to me that at a minimum there should be a final spec that is
solely "here's what it will be", as opposed to the original proposal (which
was fine for that stage) which mixed two proposals. Again, if the final
proposal is 100% pure Morty from the original RFC, fine, but it would help
to have some clarity on the status of the changes that were raised during
discussion. As it stands, the direction as stated most recently above is
"stated preference for Morty", which does make it sound as if all the other
changes are definitively rejected.

—
Reply to this email directly or view it on GitHub
#4121 (comment)
.

[knz]

@JackKrupansky as you have seen the Github PR is a process whereby the document has undergone several iterations and changes. Most of the discussion has been processed and integrated in the RFC already, except for the last point which you outline about which decision is being taken. If you think there are still open questions beyond that after considering the latest version of the RFC in the PR we could take them onboard.

See @andreimatei's response for the technical specific bit.

[JackKrupansky]

Okay, thanks. I guess my problem was that I looked briefly at the RFC doc a second time and didn't see any apparent changes right away and assumed that it had not yet been updated. I see now that there was a second commit with changes. Thanks again.

In any case, don't let me slow down the Beta implementation.