Allow collation to be a part of data type #7748

aafemt · 2023-09-13T10:47:31Z

As discussed in https://groups.google.com/d/msgid/firebird-devel/0803eb29-46ef-41ff-a1c7-768ea69e7988%40lawinegevaar.nl

…d default collation

AlexPeshkoff · 2023-09-26T17:29:20Z

I see nothing bad that we get closer to SQL standard.

dyemanov · 2023-09-26T17:39:46Z

A few comments after looking at parse.y:

By SQL spec, COLLATE can be specified not only for specific character set, but also for national_character_type.
I see charset_clause also in udf_data_type and array_type, they're not covered by this patch.

Also, accordingly to the SQL spec, COLLATE cannot be specified for BINARY. It corresponds to your new grammar, but what about CHARACTER SET OCTETS being specified explicitly with some COLLATE? Is it going to be rejected due to collation mismatch after INTL tables lookup or maybe we should prohibit it with a dedicated error message?

dyemanov · 2023-09-26T17:51:50Z

doc/sql.extensions/README.ddl.txt

+24) COLLATE clause can be used as a part of character data type as per SQL standard.
+(Dmitry Sibiryakov)
+
+If is used twice, an error is returned.


This does not correspond to changes below. For parameters you seem to allow both and use the latest one. What about local variables?

This PR is intended for table fields only, I'm not sure if parameters and variables are covered. Should they?

You have added the code to ParameterClause::ctor with a comment:
// Explicit collation has precedence over data type collation
This does not match the text about error being raised.

And parameters/variables are declared using:
column_domain_or_non_array_type collate_clause ...
where column_domain_or_non_array_type now includes a collate_clause itself after your changes...
domain_or_non_array_type -> domain_or_non_array_type_name -> non_array_type -> simple_type

This comment applies to cases when data type is derived from domain or column including collation. Old code silently reset collation to default if no explicit COLLATE was provided so I've added the condition and the comment.

For parameters you seem to allow both and use the latest one.

Indeed, I'll fix that but it is not really related to the comment.

What about local variables?

Local variables seems to be ok:

SQL> create procedure p2 as CON> declare a varchar(10) character set win1251 collate pxw_cyrl not null; CON> declare b varchar(10) character set win1251 not null collate pxw_cyrl; CON> declare c vu collate unicode_ci_ai; CON> declare d vu; CON> declare e vc not null collate win1251; CON> declare f vc; CON> begin end^ SQL> create procedure p3 as CON> declare a varchar(10) character set win1251 collate pxw_cyrl not null collate win1251; CON> begin end^ Statement failed, SQLSTATE = 42000 SQL error code = -637 -duplicate specification of COLLATE - not supported

It is pity that setClause() is not available outside of parser. Can they be made static?

IMHO, they (along with setClauseFlag / checkDuplicateClause / isDuplicateClause) should be static, as they don't access the parser state. @asfernandes , do you agree?

I would question the need in collate_clause after domain_or_non_array_type and column_domain_or_non_array_type, as now it's also included inside these parser rules. The only regression would be that NOT NULL must be written after COLLATE, not before as now. Is it an acceptable breakage, given the low probability that both co-exist in input/output parameters / return values in real-world scripts? And there will be no need to check for duplicated definitions then.

If nodes are going to call Parser::setClause, I don't agree. It's layer violation.
In this case I would move them to Node class.

Collation can be removed from ParameterClause constructor parameters and assigned later. Is it ok?

aafemt · 2023-09-26T19:24:44Z

array_type is not covered on purpose for two reasons:

arrays are not much supported in general;
it adds another parser conflict that is not good.

dyemanov

Basically, I agree with the PR, but the questioned issues must be addressed before merging.

dyemanov · 2023-09-27T07:15:22Z

array_type is not covered on purpose for two reasons:
1. arrays are not much supported in general;
2. it adds another parser conflict that is not good.

I don't mind.

aafemt · 2023-09-27T14:02:47Z

but what about CHARACTER SET OCTETS being specified explicitly with some COLLATE? Is it going to be rejected due to collation mismatch after INTL tables lookup or maybe we should prohibit it with a dedicated error message?

Charset OCTETS has only one collation so unless somebody created a custom collation for it (is it possible at all?) you'll get error "collation not found" trying to set collation to anything but default "OCTETS". I don't think that using of COLLATE should be prohibited for particular charset(s). At least for the sake of backward compatibility.

dyemanov · 2023-09-28T07:27:24Z

but what about CHARACTER SET OCTETS being specified explicitly with some COLLATE? Is it going to be rejected due to collation mismatch after INTL tables lookup or maybe we should prohibit it with a dedicated error message?

Charset OCTETS has only one collation so unless somebody created a custom collation for it (is it possible at all?) you'll get error "collation not found" trying to set collation to anything but default "OCTETS". I don't think that using of COLLATE should be prohibited for particular charset(s). At least for the sake of backward compatibility.

OK, fine with me.

dyemanov · 2023-09-28T08:08:10Z

src/dsql/parse.y

@@ -4737,14 +4737,16 @@ array_range
 %type <legacyField> simple_type
 simple_type
 	: non_charset_simple_type
-	| character_type charset_clause
+	| character_type charset_clause collate_clause
 		{


I'm not sure whether it works in our codebase, but accordingly to the SQL specification collate_clause must also be supported for text blobs.

Yes, but Firebird has no standard character blobs (CLOB or CHARACTER LARGE OBJECT), only binary blobs so this part is not for this PR.

aafemt · 2023-09-28T15:13:11Z

By SQL spec, COLLATE can be specified not only for specific character set, but also for national_character_type.

Added.

asfernandes · 2024-09-11T01:58:44Z

@aafemt this change introduced a problem making this query work:

select cast('x' as varchar(10) character set utf8 collate wrong) from rdb$database;

aafemt · 2024-09-11T17:45:36Z

If something that never worked started to work I wouldn't call it "a problem".
If cast ignores collation in target data type definition it is the problem with cast and not data type definition. Thus I extract your comment to a separate issue.

asfernandes · 2024-09-12T00:24:21Z

If something that never worked started to work I wouldn't call it "a problem".

Not true. It worked perfectly well, giving an error in a not supported contruction.

If cast ignores collation in target data type definition it is the problem with cast and not data type definition. Thus I extract your comment to a separate issue.

No. It's a problem introduced in this PR.

aafemt · 2024-09-12T09:31:56Z

It worked perfectly well, giving an error in a not supported contruction

Now the construction is supported and the query doesn't produce "not supported" error. It is logical and usual behavior of any new functionality.

asfernandes · 2024-09-12T10:30:58Z

Now the construction is supported and the query doesn't produce "not supported" error. It is logical and usual behavior of any new functionality.

I've no idea of what are you talking, really. Makes no sense for me.

Is it a way to say that you're not going to fix regression #8249 introduced by this your change?

aafemt · 2024-09-12T10:45:40Z

Is it a way to say that you're not going to fix regression #8249 introduced by this your change?

It is a way to tell you "do not call this regression". Regression is a case when working documented functionality doesn't work anymore.

#8249 is a separate bug that should be fixed by me or anyone else like any other bug. You assigned it to me - fine. Now there is nothing to do but to sit and wait.

aafemt added 2 commits September 13, 2023 12:44

Allow collation to be a part of data type

0d2ad6d

Merge branch 'master' into dt_collate

3c8b5e0

aafemt referenced this pull request Sep 26, 2023

Fix character set and collation output for character sets with altere…

72f55f2

…d default collation

AlexPeshkoff self-requested a review September 26, 2023 17:26

AlexPeshkoff approved these changes Sep 26, 2023

View reviewed changes

dyemanov reviewed Sep 26, 2023

View reviewed changes

dyemanov requested changes Sep 27, 2023

View reviewed changes

Error on duplicate COLLATE clause in routine parameters

24e8732

dyemanov reviewed Sep 28, 2023

View reviewed changes

Add collation to national character string type

1df21d2

aafemt added 4 commits September 29, 2023 16:42

Move collation assignment into parser for uniform error handling

daa01f5

Merge branch 'master' into dt_collate

6f175cd

Code simplification and memory leak prevention

83916a3

Merge branch 'master' into dt_collate

7fab551

aafemt requested a review from dyemanov November 11, 2023 14:04

dyemanov approved these changes Nov 18, 2023

View reviewed changes

Merge branch 'master' into dt_collate

9b66e2b

AlexPeshkoff merged commit e23f030 into FirebirdSQL:master Nov 24, 2023
22 checks passed

pavel-zotov added the qa: done successfully label Nov 24, 2023

aafemt mentioned this pull request Sep 11, 2024

CAST() ignores collation of target data type #8249

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow collation to be a part of data type #7748

Allow collation to be a part of data type #7748

aafemt commented Sep 13, 2023

AlexPeshkoff commented Sep 26, 2023

dyemanov commented Sep 26, 2023 •

edited

Loading

dyemanov Sep 26, 2023

aafemt Sep 26, 2023

dyemanov Sep 27, 2023 •

edited

Loading

aafemt Sep 27, 2023

aafemt Sep 27, 2023

aafemt Sep 27, 2023

dyemanov Sep 28, 2023

dyemanov Sep 28, 2023

asfernandes Sep 28, 2023

aafemt Sep 28, 2023

aafemt commented Sep 26, 2023

dyemanov left a comment

dyemanov commented Sep 27, 2023

aafemt commented Sep 27, 2023

dyemanov commented Sep 28, 2023

dyemanov Sep 28, 2023

aafemt Sep 28, 2023

aafemt commented Sep 28, 2023

asfernandes commented Sep 11, 2024

aafemt commented Sep 11, 2024

asfernandes commented Sep 12, 2024

aafemt commented Sep 12, 2024

asfernandes commented Sep 12, 2024

aafemt commented Sep 12, 2024

Allow collation to be a part of data type #7748

Allow collation to be a part of data type #7748

Conversation

aafemt commented Sep 13, 2023

AlexPeshkoff commented Sep 26, 2023

dyemanov commented Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyemanov Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aafemt commented Sep 26, 2023

dyemanov left a comment

Choose a reason for hiding this comment

dyemanov commented Sep 27, 2023

aafemt commented Sep 27, 2023

dyemanov commented Sep 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aafemt commented Sep 28, 2023

asfernandes commented Sep 11, 2024

aafemt commented Sep 11, 2024

asfernandes commented Sep 12, 2024

aafemt commented Sep 12, 2024

asfernandes commented Sep 12, 2024

aafemt commented Sep 12, 2024

dyemanov commented Sep 26, 2023 •

edited

Loading

dyemanov Sep 27, 2023 •

edited

Loading