[CALCITE-6852] Project's digest calculation error, should include FieldNames in rowType

[xiedeyantu]

No description provided.

[mihaibudiu]

does this match what happens for other operators?

[xiedeyantu]

does this match what happens for other operators?

@mihaibudiu Perhaps other operators need to check the deepHashCode0. But the original deepHashCode0 of the project may lead to incorrect choices. This change may expose errors in two rules, but I have fixed them.

[mihaibudiu]

There are two reasons to check what is done in other similar places:

• if this is a bug, we should try to find out whether the bug appears in other places so we can fix it. Not in this PR, but at least we can file a new issue about it.
• if no other place checks field names, perhaps the intent of the digest is to ignore field names, and then maybe the bug is elsewhere

[mihaibudiu]

Note that I personally don't know what the intent of the digest is.

[xiedeyantu]

Note that I personally don't know what the intent of the digest is.

please see this issue: https://issues.apache.org/jira/projects/CALCITE/issues/CALCITE-6852

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[mihaibudiu]

I saw the issue, and I agree that there is a bug, but the question I am asking is whether this is the bug or the bug is elsewhere.
Perhaps someone who knows the intent of the digest can comment. I didn't try to investigate this myself by reading the code for digest in other classes.

[xiedeyantu]

I saw the issue, and I agree that there is a bug, but the question I am asking is whether this is the bug or the bug is elsewhere. Perhaps someone who knows the intent of the digest can comment. I didn't try to investigate this myself by reading the code for digest in other classes.

@mihaibudiu OK, I think this is indeed a summary calculation problem of the project. The corresponding relationship between the operator and Vertex will be recorded in mapDigestToVertex, which makes it easy to retrieve the corresponding operator in findBestExp. If the Project loses the field name information during calculation, it will be deduplicated by map and replaced by a project with the same field but only a different alias. This may depend on the order of the put operators. As for other operators, the only one I can think of is agg, because only operators with column clipping may be involved in this problem, but generally agg will have a project on it, so the overall plan may be fine.

[xiedeyantu]

I saw the issue, and I agree that there is a bug, but the question I am asking is whether this is the bug or the bug is elsewhere. Perhaps someone who knows the intent of the digest can comment. I didn't try to investigate this myself by reading the code for digest in other classes.

Hi, @mihaibudiu , I have added another part of the problem explanation. Please take a look at this CALCITE-6852 again

[rubenada]

It seems to me that this comment in Project#explainTerms suggests that what we are discussing here was actually intended:

  @Override public RelWriter explainTerms(RelWriter pw) {
    ...
    // Skip writing field names so the optimizer can reuse the projects that differ in
    // field names only

[xiedeyantu]

It seems to me that this comment in Project#explainTerms suggests that what we are discussing here was actually intended:

  @Override public RelWriter explainTerms(RelWriter pw) {
    ...
    // Skip writing field names so the optimizer can reuse the projects that differ in
    // field names only

@rubenada Yes, the problem should arise here, missing aliases is a very serious problem. But we can't modify this, it will cause a big change in behavior.