[CALCITE-7085] JOIN USING with unqualified common column fails in a conformance where allowQualifyingCommonColumn is false (e.g. Oracle, Presto)

[cjj2010]

jira: https://issues.apache.org/jira/browse/CALCITE-7085

[mihaibudiu]

This does not look like a robust way to carry information between compilation stages; in general, the parser position is best-effort in Calcite.

There must be a better way to do this. My intuition tells me that the unparser has to handle this case by stripping out the qualifying information. Alternatively, can the join type be changed to an INNER JOIN?

[cjj2010]

Thank you for your suggestion. The original plan was indeed inadequate. I have now revised it to completely separate validation from expansion — validating the original AST once before expansion, and not performing validation during expansion.

[mihaibudiu]

But why remove the position from the identifier?

[cjj2010]

why remove the position from the identifier?

Thank you for reviewing this pull request (PR). The identifier here is synthetic — the validator fabricates EMP.DEPTNO / DEPT.DEPTNO when expanding JOIN ... USING(deptno) into COALESCE(EMP.DEPTNO, DEPT.DEPTNO) AS DEPTNO. The user never typed it.
Three reasons to use SqlParserPos.ZERO:

1. SqlParserPos.ZERO is documented as "use this if the node doesn't correspond to a position in piece of SQL text." The surrounding COALESCE and AS in this same expansion already use ZERO; the inner identifiers should match.
2. With identifier.getParserPosition(), the synthetic EMP.DEPTNO was indistinguishable from a user-written one. When it re-entered expandCommonColumn, conformances with allowQualifyingCommonColumn() == false (Oracle, Presto, …) raised a spurious Cannot qualify common column 'EMP.DEPTNO'. Pairing this change with the new pre-expansion check validateNoQualifiedCommonColumns cleanly separates "user wrote it" (real position, rejected) from "validator synthesized it" (ZERO, allowed).
3. Any future error raised on this node would otherwise underline the user's deptno and report EMP.DEPTNO — misleading for SqlAdvisor.

[mihaibudiu]

I don't understand. Are you saying that having position on the identifier leads to an error, and not having one does not? I didn't know that's possible.

What is a "synthetic" identifier? In the end all identifiers come from the source code.

Providing errors without source position is a worse user-experience, even if the program has been optimized. Is the concern for SqlAdvisor based on a concrete example, or is it hypothetical?

[cjj2010]

Thanks for the careful review — these are good questions.
On the SqlParserPos.ZERO change:
The SqlIdentifier nodes created inside expandExprFromJoin (i.e., the t1.col and t2.col sub-expressions that form the COALESCE) are not parsed from user input — they are constructed programmatically by the expansion logic. The user only writes col; the framework builds the qualified forms internally. Since validation is now performed before expansion, directly on the original user-written identifiers (which carry proper source positions), these internally generated nodes no longer need to carry a meaningful position. Using identifier.getParserPosition() on them was misleading — it implied they were the same node as the original identifier, which they are not. SqlParserPos.ZERO is the standard convention in Calcite for internally generated nodes (see also expandStar, validateFrom, etc.).
On "synthetic identifier":
I used "synthetic" informally to mean "created programmatically, not parsed from user SQL." I agree the term is not ideal — I'll remove it from the comment to avoid confusion.
On the SqlAdvisor concern:
The concern was hypothetical, not based on a concrete failing case. SqlAdvisorValidator overrides expand, expandSelectExpr, and expandOrderExpr to return expressions unchanged, so expandExprFromJoin is not reachable from that path. More importantly, error reporting quality is not degraded: validation errors are now thrown on the original user-written identifiers (before expansion), which always carry real source positions. So users will still see accurate error locations.

[mihaibudiu]

Validation errors are not the only errors we have to be concerned about.
Our compiler carries the position information all the way to the runtime, and surfaces runtime errors using source position information. A runtime error without position information usually provides very little actionable insight to the user about what went wrong. So in general I am trying very hard to preserve any source position information that is available.

[mihaibudiu]

I am ok to merge this as it is, but I still don't understand why you need to change the position. Does it address the issue being resolved in any way?

[cjj2010]

Sorry, I didn't reply to your message in a timely manner yesterday. After further debugging this part of the code, I believe what you said is correct. For this fix, changing SqlParserPos.ZERO is unnecessary. The core fix is to move the validation before the extension (via validateSelectCommonColumns / validateNoQualifiedCommonColumns), so the extension code no longer requires validation. I have reverted the location change - the identifier after extension will continue to carry the source location of the original identifier, which retains useful context information for runtime error reporting.

[mihaibudiu]

This looks fine, but please address my two questions

[mihaibudiu]

But why remove the position from the identifier?

[mihaibudiu]

Let's merge this, please squash the commits.
I appreciate your patience with the review process.

[cjj2010]

Let's merge this, please squash the commits. I appreciate your patience with the review process.

done, thanks

[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