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

Fixes gh-2189 Field type UUID is now available in SQL #2201

Merged
merged 6 commits into from Jul 8, 2021

Conversation

pgulutzan
Copy link
Contributor

In fact the intent is to fix gh-2189 and other issues that are duplicates or closely related because they're about sql + uuid:
gh-2151 gh-2176 gh-2177 gh-2190.
Also a few punctuation errors are fixed.
I added a label 2.9.1 -- this appears to be the relevant version but there are no release notes for it yet.
Some issues related to sql + uuid are being discussed in tarantool/tarantool#6164 comments but I don't expect them to affect documentation immediately.

@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2189 June 22, 2021 21:16 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2189 June 22, 2021 21:19 Inactive
@NickVolynkin
Copy link
Contributor

@pgulutzan thank you! Please keep this PR open, we'll translate it right away and then merge both the doc updates and the translation.

@alexandra-mara alexandra-mara self-assigned this Jun 23, 2021
@tsafin tsafin self-requested a review June 23, 2021 10:39
@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2189 June 24, 2021 15:24 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2189 June 24, 2021 15:26 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2189 June 28, 2021 16:59 Inactive
@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2189 June 28, 2021 17:00 Inactive
Copy link

@ImeevMA ImeevMA left a comment

Choose a reason for hiding this comment

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

LGTM

@xuniq xuniq requested review from xuniq and removed request for xuniq June 29, 2021 15:50
@NickVolynkin NickVolynkin assigned NickVolynkin and unassigned xuniq Jun 30, 2021
@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2189 June 30, 2021 17:33 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2189 June 30, 2021 17:33 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2189 July 2, 2021 10:33 Inactive
@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2189 July 6, 2021 12:04 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2189 July 6, 2021 12:04 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2189 July 6, 2021 12:08 Inactive
@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2189 July 6, 2021 12:08 Inactive
Copy link
Collaborator

@igormunkin igormunkin left a comment

Choose a reason for hiding this comment

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

@pgulutzan, thanks for the changes, LGTM, except the one question: I found no changes related to SCALAR comparison order mentioned in #2151. Is there a place, where this order is described?

@@ -485,8 +485,9 @@ Data types may also appear in :ref:`CAST <sql_function_cast>` functions.
column7 STRING, column8 STRING COLLATE "unicode",
column9 TEXT, columna TEXT COLLATE "unicode_sv_s1",
columnb VARCHAR(0), columnc VARCHAR(100000) COLLATE "binary",
columnd VARBINARY,
columne SCALAR, columnf SCALAR COLLATE "unicode_uk_s2");
columnd UUID,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor: why is this field added as a non-last attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not added as a non-last attribute. The text that I see in the changed file looks like this:
-- with all possible data types and aliases
CREATE TABLE t
(column1 BOOLEAN, column2 BOOL,
column3 INT PRIMARY KEY, column4 INTEGER,
column5 DOUBLE,
column6 NUMBER,
column7 STRING, column8 STRING COLLATE "unicode",
column9 TEXT, columna TEXT COLLATE "unicode_sv_s1",
columnb VARCHAR(0), columnc VARCHAR(100000) COLLATE "binary",
columnd UUID,
columne VARBINARY,
columnf SCALAR, columng SCALAR COLLATE "unicode_uk_s2");

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I see. And the question is why did you change collumnd type to UUID, instead of making the new attribute collumng of UUID type? Anyway, I am totally OK with this change, just trying to get your point regarding it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I misunderstood your question, sorry. For this example we can put the columns in any order. I was not trying to make a point. I am wondering now: is there any way that putting columns in a different order affects performance? But, regardless what the answer for that is, it won't matter for this example.


Return a Universal Unique Identifier, data type UUID.
Optionally one can specify a version number; however, at this time the
only allowed version is 4, which is the default.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor: IMHO, "supported" fits much better here than "allowed" does. Feel free to ignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realize that if I say UUID(3) I get an error message that includes the word "supported".
However, the fact that I get an error message means that UUID(3) is not allowed.
If I had not gotten an error message, that would mean UUID(3) is allowed but meaningless
and probably will have no effect -- that would be my idea of "not supported". Understandable?

@pgulutzan
Copy link
Contributor Author

@igormunkin: your question shows that I was careless when looking at #2151, I did not see that it was not marked as an SQL issue.
Re SQL ordering: I could add ", then UUIDs" at the end of this line:
https://www.tarantool.io/en/doc/latest/reference/reference_sql/sql_statements_and_clauses/#order-by-clause
"NULLs come first, then BOOLEANs, then numbers, then STRINGs, then VARBINARYs, then UUIDs."
but the significant changes are not in the SQL section of the manual, they are in the Lua section.
https://www.tarantool.io/en/doc/latest/book/box/data_model/
"When a scalar field contains values of different underlying types, the key order is: nils, then booleans, then numbers, then strings, then varbinaries."
and
https://www.tarantool.io/en/doc/latest/reference/reference_lua/box_space/create_index/
"When there is a mix of types, the key order is: null, then booleans, then numbers, then strings, then byte arrays"
In both cases ", then uuids" should be added.
@NickVolynkin: should I treat the above changes as a separate issue and make a new pull request for #2151, or add a commit for this pull request?

@igormunkin
Copy link
Collaborator

@igormunkin: your question shows that I was careless when looking at #2151, I did not see that it was not marked as an SQL issue.

No problem. I was simply confused with the message, that #2151 will be also resolved in scope of this PR.

Re SQL ordering: I could add ", then UUIDs" at the end of this line:
https://www.tarantool.io/en/doc/latest/reference/reference_sql/sql_statements_and_clauses/#order-by-clause
"NULLs come first, then BOOLEANs, then numbers, then STRINGs, then VARBINARYs, then UUIDs."

Agree.

but the significant changes are not in the SQL section of the manual, they are in the Lua section.
https://www.tarantool.io/en/doc/latest/book/box/data_model/
"When a scalar field contains values of different underlying types, the key order is: nils, then booleans, then numbers, then strings, then varbinaries."
and
https://www.tarantool.io/en/doc/latest/reference/reference_lua/box_space/create_index/
"When there is a mix of types, the key order is: null, then booleans, then numbers, then strings, then byte arrays"
In both cases ", then uuids" should be added.

Agree.

@NickVolynkin: should I treat the above changes as a separate issue and make a new pull request for #2151, or add a commit for this pull request?

IMHO, it would be better to resolve #2151 in a separate PR, since it relates to SCALAR and UUID for both language runtimes. I also summon @ImeevMA here as an author of the ticket and the patch.

@pgulutzan
Copy link
Contributor Author

@igormunkin, I think that you are right. If @NickVolynkin agrees, let us say:
All reviews of this pull request have passed except the one from @tsafin,
and it fixes all the issues that I mentioned except issue#2151,
so we can end it soon.
Issue#2151 will not be fixed in the scope of this PR, I will make a separate pull request.
Incidentally I looked more closely and saw two more things in
https://www.tarantool.io/en/doc/latest/book/box/data_model/
"
scalar. Values in a scalar field can be boolean or integer or unsigned or double or number or decimal or string or varbinary – but not array or map or tuple. Examples: true, 1, 'xxx'.
"
..."
any. Values in an any field can be boolean or integer or unsigned or double or number or decimal or string or varbinary – or array or map or tuple. Examples: true, 1, 'xxx', {box.NULL, 0}.
"
This will be (I hope) easy for all of us.

@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2189 July 8, 2021 05:45 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2189 July 8, 2021 05:45 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2189 July 8, 2021 05:58 Inactive
@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2189 July 8, 2021 05:59 Inactive
@NickVolynkin NickVolynkin merged commit fd4bb77 into latest Jul 8, 2021
@NickVolynkin NickVolynkin deleted the pgulutzan-gh-2189 branch July 8, 2021 05:59
@NickVolynkin NickVolynkin restored the pgulutzan-gh-2189 branch July 8, 2021 06:01
@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2189 July 8, 2021 06:04 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2189 July 8, 2021 06:04 Inactive
@NickVolynkin NickVolynkin deleted the pgulutzan-gh-2189 branch July 8, 2021 06:08
@NickVolynkin NickVolynkin restored the pgulutzan-gh-2189 branch July 8, 2021 06:10
@NickVolynkin NickVolynkin deleted the pgulutzan-gh-2189 branch July 8, 2021 06:12
@NickVolynkin
Copy link
Contributor

@pgulutzan and @alexandra-mara, thank you for all the good work. I've just merged the PR.

I made a mistake with the first merge and had to re-merge it.. twice. As a result, the commit is made under my name, but I've explicitly stated the authorship of the original text and the translation. I apologize for the inconvenience this might have caused.

@pgulutzan
Copy link
Contributor Author

@NickVolynkin, for me that is the opposite of inconvenient, since I guess it means we have now finished with this pull request and I will be able to close all the related issues assigned to me -- except issue#2151, since as discussed with @igormunkin above I have a few more words. See issue#2235 Fixes gh-2151 UUID is now part of SCALAR.

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.

Field type UUID is now available in SQL
6 participants