[CALCITE-5988] SqlImplementor.toSql cannot emit VARBINARY literals

[mihaibudiu]

No description provided.

[mihaibudiu]

@julianhyde I think you wrote most of this code (long time ago), perhaps you can review this tiny PR? Thank you.

[julianhyde]

Please don't ask me, personally, to review PRs. I have done a huge amount for this project. That doesn't give everyone the right to ask me to do more.

I reviewed a PR yesterday because you said I'd written the code. That wasn't true. I had merely refactored the test.

[libenchao]

@mihaibudiu Thanks for your PR, it looks good generally, I've left some minor comments.

[libenchao]

NIT: why not use castNonNull to keep it the same with other types?

[libenchao]

I'm wondering if select cast(null as binary) would work for current implementation. It's true that all other types does not consider null, they may suffer from the same problem.

[libenchao]

I'm wondering if select cast(null as binary) would work for current implementation. It's true that all other types does not consider null, they may suffer from the same problem.

@mihaibudiu What's your thought on this comment?

[mihaibudiu]

I have added a test for this. It works.

[libenchao]

Seems that the SqlTypeName of a RexLiteral is always NULL, no matter what the RelDataType is.

[mihaibudiu]

I think it's much worse, the typeName of RexLiteral is not null, it is used in a very ad-hoc way. It almost never matches the actual type.

The field's Javadoc discusses this:

// TODO jvs 26-May-2006:  Use SqlTypeFamily instead; it exists
  // for exactly this purpose (to avoid the confusion which results
  // from overloading SqlTypeName).
  /**
   * An indication of the broad type of this literal -- even if its type isn't
   * a SQL type. Sometimes this will be different than the SQL type; for
   * example, all exact numbers, including integers have typeName
   * {@link SqlTypeName#DECIMAL}. See {@link #valueMatchesType} for the
   * definitive story.
   */

So the typeName is used instead of a TypeFamily. I personally think this field should be completely removed, but there are many uses in the code that would have to be audited. It should have been deprecated in 2006. Moreover, removing it may break third-party software that depends on Calcite that is not tested as part of the Calcite testing process. The type is the right field to use.

The TypeFamily itself has a very unclear meaning: https://issues.apache.org/jira/browse/CALCITE-5986

I have tried to clean up the TypeFamily in #3411, but I couldn't do it systematically, it's now also used itself in many places.

[libenchao]

how about name it testBinaryLiteral, it's more precise.

[libenchao]

Is withMysql necessary to reproduce the issue, if not, I'd suggest to just use sql(query).ok(exptected)

[mihaibudiu]

I have added a comment explaining this choice, but if you prefer to use the default Calcite dialect I can change the test.

[libenchao]

I'm fine with leaving as it is.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

100.0% Coverage
0.0% Duplication

[libenchao]

+1, merging