[CALCITE-7339] Most classes in SqlDdlNodes use an incorrect SqlCallFactory

[mihaibudiu]

I fixed most classes, but I don't know how to fix the SqlAttributeDefinition fully.

[xiedeyantu]

I don't understand this place very well. It would be great if @dssysolyatin could help to take a look.

[dssysolyatin]

@mihaibudiu You decided not to wait for the PR #4644 and did the work yourself. That’s totally understandable. I’m sorry I couldn’t finish my PR in time, so I understand why you moved forward.

If possible, please use SqlBasicOperator. See the discussion here:
https://issues.apache.org/jira/browse/CALCITE-5403 (especially this comment)

Having a subclass of SqlSpecialOperator for every kind of Ast node is a pattern that we need to stop doing. Can you find a way to do this using SqlBasicOperator or similar?

If method is private:

private static final SqlSpecialOperator OPERATOR

we can change SqlSpecialOperator to SqlOperator

private static final SqlOperator OPERATOR

and use SqlBasicOperator instead. See example inside #4644

Regarding SqlAttributeDefinition:

1. I don’t see where SqlCollation is used for SqlAttributeDefinition. In the parser, the only place that references it passes null as SqlCollation. So, I assume nobody uses it.
2. If we decide to support it, I think SqlCollation should extend SqlNode directly. It even already has an unparse method. For Coercibility, you can use SqlLiteral.createSymbol().

[mihaibudiu]

I tried to use these classes and I couldn't.
I think we can use any operator as long as we override the method to create a call.
I am not sure it's enough to have collation extend SqlNode; wouldn't some visitors need to know about it too?
I see that Collation in attributes is never set, but maybe it's used in derived classes.
One can convert enums to SqlLiterals with string values. But how about the Locale in the Collation?

[mihaibudiu]

So @dssysolyatin your recommendation is to switch to use SqlOperator everywhere?
What other changes are necessary?

[dssysolyatin]

Recommendation is to use SqlBasicOperator instead of making new subclasses of SqlSpecialOperator. Example https://github.com/apache/calcite/pull/4644/files#diff-0e0b4e3b10ec6fc8fcd1910b4d2b070e0c0b5a62d5bd75befbd46d3d7d33712c

 private static final SqlOperator OPERATOR =
      SqlBasicOperator.create("ATTRIBUTE_DEF", SqlKind.ATTRIBUTE_DEF)
          .withCallFactory((operator, functionQualifier, pos, operands) ->
              new SqlAttributeDefinition(
                  pos,
                  (SqlIdentifier) requireNonNull(operands[0]),
                  (SqlDataTypeSpec) requireNonNull(operands[1]),
                  operands[2], null));

I tried to use these classes and I couldn't.

Do you mean you tried SqlBasicOperator and it didn’t work as expected?

to switch to use SqlOperator everywhere?

In current cases, all OPERATORs are private (For public properties, it would be better to use just SqlOperator to avoid possible compatibility issues in the future). Therefore, if SqlBasicOperator is not used, there is no real need to change this.

One can convert enums to SqlLiterals with string values. But how about the Locale in the Collation?

Is a simple string insufficient to represent a Locale and Collation? I think it what one of the constructor does:

public SqlCollation(
      @JsonProperty("collationName") String collation,
      @JsonProperty("coercibility") Coercibility coercibility) {

[mihaibudiu]

Indeed, I forgot about #4644; this is a subset of the work in #4644, and has nothing to do with unparse. You can think of this as half of #4644. We run into problems with visitors for these classes, so I would like to solve this before #4644 is finished.

[mihaibudiu]

@dssysolyatin turns out that if you switch from SqlSpecialOperator to SqlBasicOperators quite a few BabelQuidem tests start failing, I didn't investigate why. But your PR may have the same issue.

[dssysolyatin]

turns out that if you switch from SqlSpecialOperator to SqlBasicOperators quite a few BabelQuidem tests start failing, I didn't investigate why. But your PR may have the same issue.

Okay, then don’t spend time on it. I’ll look into it separately

[dssysolyatin]

Does this test tests everything ?
Can we just add common test SqlParserTest.java but have boolean flag which enable it only for ServerUnParserTest ?

[mihaibudiu]

I don't think that my changes affect parsing and unparsing, they affect for sure visitors that mutate these classes.

[dssysolyatin]

This is one way to check that all operators have createCall and that it creates SqlCall correctly.

1. Parse the SQL string into a SqlNode.
2. Unparse it and save the result as sql1.
3. Parse the SQL again.
4. Apply a shuttle that makes a deep copy of the tree.
5. Unparse copied tree and check that the SQL string is the same as in step 2.

This way, we do not need to write a separate test for each SqlOperator. Having separate tests is also good, but with separate tests we might forget to write one and miss validating some cases.

Anyway, you can just skip this part as separate tests are also OK for me.

[mihaibudiu]

@dssysolyatin I hope I have made the changes you suggested.

[dssysolyatin]

This is one way to check that all operators have createCall and that it creates SqlCall correctly.

1. Parse the SQL string into a SqlNode.
2. Unparse it and save the result as sql1.
3. Parse the SQL again.
4. Apply a shuttle that makes a deep copy of the tree.
5. Unparse copied tree and check that the SQL string is the same as in step 2.

This way, we do not need to write a separate test for each SqlOperator. Having separate tests is also good, but with separate tests we might forget to write one and miss validating some cases.

Anyway, you can just skip this part as separate tests are also OK for me.

[mihaibudiu]

I see your point, that would have been simpler, but since I did it, I will probably leave it like this. At least it's obvious what is being tested.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
69.3% Coverage on New Code
2.2% Duplication on New Code

See analysis details on SonarQube Cloud