[CALCITE-6146] Target charset should be used when comparing two strings through CONVERT/TRANSLATE function during validation by ILuffZhe · Pull Request #3558 · apache/calcite

ILuffZhe · 2023-12-02T07:42:27Z

No description provided.

mihaibudiu · 2023-12-02T18:55:00Z

    return type;
  }

+  @Nullable Charset getCharsetAfterConvert(SqlBasicCall call, @Nullable Charset typeCharset) {


Are there other functions which should preserve charsets?
For example, if you do string concatenation, or cast from CHAR to VARCHAR?

As far as I know, only CONVERT and TRANSLATE function explicitly use charset.
In MySQL, CAST function can be used like cast(name as CHAR CHARACTER SET utf8), but we don't support such form currently.

I was asking about something like CAST(TRANSLATE("name" using BIG5) AS CHAR(5)).
Is the charset of the result of this expression correct?

The char(5) has the charset ISO-8859-1 preserved in SqlCollation, so the charset of CAST(TRANSLATE("name" using BIG5) AS CHAR(5)) should also be ISO-8859-1.
I add a test for this, please take a look.

I was asking about something like CAST(TRANSLATE("name" using BIG5) AS CHAR(5)). Is the charset of the result of this expression correct?

The result charset of TRANSLATE("name" using BIG5) is BIG5, so the cast to CHAR(5) with ISO-8859-1 is not allowed.

mihaibudiu · 2023-12-03T01:05:53Z

+    with.query("select \"name\", \"empid\" from \"hr\".\"emps\"\n"
+        + "where cast(convert(\"name\" using GBK) as char(5))=_BIG5'Eric'")
+        .throws_(
+            "Cannot apply = to the two different charsets ISO-8859-1 and Big5");


I know you didn't write this error message, but I think it can be improved.
I would write "Cannot apply operation '=' to strings with different charsets 'ISO-8859-1' and 'Big5'`

Have you tried a similar test as a SqlOperatorTest as well, just to make sure that it triggers there as well?
I don't think this change is JDBC specific.

I know you didn't write this error message, but I think it can be improved. I would write "Cannot apply operation '=' to strings with different charsets 'ISO-8859-1' and 'Big5'`

Agreed, and I've changed the message.

Have you tried a similar test as a SqlOperatorTest as well, just to make sure that it triggers there as well? I don't think this change is JDBC specific.

I've added some tests in SqlOperatorTest, however the tests testStringComparisonWithConvertFunc in JdbcTest still fail.
When creating a new JavaType with different charset(by createTypeWithCharsetAndCollation) during validation, the wrapped charset in this JavaType is overwritten through canonizing process due to the DATATYPE_CACHE.
I'm not sure this is a bug or specific designed, and I think considering the charset for JavaType's(java.lang.String) digest computing might be a way to go.

mihaibudiu · 2023-12-03T01:12:00Z

    if (SqlTypeUtil.inCharFamily(operandType0)
        && SqlTypeUtil.inCharFamily(operandType1)) {
      Charset cs0 = operandType0.getCharset();
+      if (call.operand(0) instanceof SqlBasicCall) {


Is this the right place for this check?
Why does the operandType0 have the wrong charset?
Maybe the bug is in the place where the charset for operand0 type was inferred.

You are right. I've added inferReturnType for both CONVERT and TRANSLATE function.

sonarqubecloud · 2023-12-03T01:32:13Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

ILuffZhe · 2024-12-05T01:06:02Z

@mihaibudiu I mark the PR to draft, and I'll let you know if anything updated in the next few days.

mihaibudiu · 2024-12-08T06:38:45Z

+          + "where cast(convert(\"name\" using LATIN1) as char(5))='Eric'")
+        .returns("name=Eric; empid=200\n");
+    with.query("select \"name\", \"empid\" from \"hr\".\"emps\"\n"
+          + "where cast(convert(\"name\" using GBK) as char(5))='Eric'")


why is this the expected result?

why is this the expected result?

The result of convert(\"name\" using GBK) has GBK charset while CHAR(5) has ISO-8859-1 charset, which is not allowed in SqlCastFunction.

That is not obvious. I would add that as a comment

OK, I've added comment in both JdbcTest and SqlOperatorTest

mihaibudiu · 2024-12-08T06:42:22Z

+
+  @Override public RelDataType deriveType(SqlValidator validator,
+      SqlValidatorScope scope, SqlCall call) {
+    // special case for TRANSLATE: don't need to derive type for Charsets


it's not obvious what this comment refers to. Special compared to what?

Got your point, I'll change it to don't need to derive type for Charsets in both CONVERT and TRANSLATE. WDYT?

mihaibudiu · 2024-12-08T06:44:27Z

+  @Test void testStringComparisonWithConvertFunc() {
+    final SqlOperatorFixture f = fixture();
+    f.setFor(SqlStdOperatorTable.CONVERT, VM_JAVA);
+    f.check("select 'a' as col\n"


this example is confusing, because the column name is col and the literal is also col.
Can you change one of them?

this example is confusing, because the column name is col and the literal is also col. Can you change one of them?

Sure, I've changed the column alia.

mihaibudiu

I have never used these functions myself, but, assuming the semantics implemented is the correct one, this PR looks fine.

Can some of these results be validated on another DB?

ILuffZhe · 2024-12-09T00:53:41Z

I have never used these functions myself, but, assuming the semantics implemented is the correct one, this PR looks fine.

Can some of these results be validated on another DB?

Thank you for the review! @mihaibudiu

In earlier's PR, we've tested CONVERT(mysql) and TRANSLATE(bigquery) in function.iq , and you can take a look if it's satisfied.

BTW, I'll try to squash the commits if no more new comments come.

…gs through CONVERT/TRANSLATE function during validation

sonarqubecloud · 2024-12-09T13:36:08Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
96.7% Coverage on New Code
38.9% Duplication on New Code

See analysis details on SonarQube Cloud

ILuffZhe force-pushed the CALCITE-6146 branch from c9f9b4d to e851afe Compare December 2, 2023 08:28

mihaibudiu reviewed Dec 2, 2023

View reviewed changes

mihaibudiu reviewed Dec 3, 2023

View reviewed changes

ILuffZhe marked this pull request as draft December 5, 2024 01:06

ILuffZhe force-pushed the CALCITE-6146 branch from 9c32a45 to 2951138 Compare December 7, 2024 08:25

ILuffZhe marked this pull request as ready for review December 7, 2024 08:26

ILuffZhe force-pushed the CALCITE-6146 branch from 2951138 to 6e9ae7e Compare December 7, 2024 08:44

mihaibudiu reviewed Dec 7, 2024

View reviewed changes

Comment thread core/src/test/java/org/apache/calcite/test/JdbcTest.java Outdated

ILuffZhe requested a review from mihaibudiu December 8, 2024 02:28

mihaibudiu reviewed Dec 8, 2024

View reviewed changes

mihaibudiu approved these changes Dec 8, 2024

View reviewed changes

[CALCITE-6146] Target charset should be used when comparing two strin…

d3f0aec

…gs through CONVERT/TRANSLATE function during validation

ILuffZhe force-pushed the CALCITE-6146 branch from d79a4a6 to d3f0aec Compare December 9, 2024 13:09

ILuffZhe added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Dec 9, 2024

ILuffZhe merged commit 4948817 into apache:main Dec 11, 2024

Conversation

ILuffZhe commented Dec 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Dec 3, 2023

Uh oh!

ILuffZhe commented Dec 5, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mihaibudiu left a comment

Choose a reason for hiding this comment

Uh oh!

ILuffZhe commented Dec 9, 2024

Uh oh!

sonarqubecloud Bot commented Dec 9, 2024

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants