xuzifu666 commented on code in PR #5240:
URL: https://github.com/apache/calcite/pull/5240#discussion_r3943035573
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -638,6 +638,107 @@ private List<String> deriveNaturalJoinColumnList(SqlJoin
join) {
getNamespaceOrThrow(join.getRight()).getRowType());
}
+ /** Expands an identifier that refers to a common column of a NATURAL or
+ * USING join into a qualified reference or, for FULL JOIN, a COALESCE
+ * expression. */
+ SqlNode expandCommonColumn(SqlSelect sqlSelect, SqlNode selectItem,
+ SelectScope scope) {
+ return expandCommonColumn(sqlSelect, selectItem, scope, true);
+ }
+
+ /** As {@link #expandCommonColumn(SqlSelect, SqlNode, SelectScope)}, but
+ * allows control over whether the result is wrapped with an {@code AS}
+ * operator to preserve the column alias. */
+ SqlNode expandCommonColumn(SqlSelect sqlSelect, SqlNode selectItem,
+ SelectScope scope, boolean wrapAlias) {
+ if (!(selectItem instanceof SqlIdentifier)) {
+ return selectItem;
+ }
+
+ final SqlNode from = sqlSelect.getFrom();
+ if (!(from instanceof SqlJoin)) {
+ return selectItem;
+ }
+
+ final SqlIdentifier identifier = (SqlIdentifier) selectItem;
+ if (!identifier.isSimple()) {
+ // Qualified identifiers (e.g. t1.col) are returned unchanged.
+ // Validation of qualified common columns is performed before expansion,
+ // in validateSelectCommonColumns, where the original user-written
+ // identifier (with its source position) is still available.
+ return selectItem;
+ }
+
+ return expandExprFromJoin((SqlJoin) from, identifier, scope, wrapAlias);
+ }
+
+ private SqlNode expandExprFromJoin(SqlJoin join, SqlIdentifier identifier,
+ SelectScope scope, boolean wrapAlias) {
+ final List<String> commonColumnNames;
+ // must be NATURAL or USING here, and cannot specify NATURAL keyword with
USING clause
Review Comment:
You're right, the comment was wrong (it was inherited from the original
code). It's not the case that only `NATURAL`/`USING` joins can reach here: an
`ON` join also reaches this method, and for those the identifier must be
returned unchanged so that regular validation reports the ambiguity error. I've
rewritten the comment to state that.
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -5325,9 +5426,19 @@ protected void validateOrderList(SqlSelect select) {
final SqlValidatorScope orderScope = getOrderScope(select);
requireNonNull(orderScope, "orderScope");
+ // Expand unqualified common columns of NATURAL or USING joins before
+ // regular expansion, which would fail with an ambiguity error.
+ final SelectScope selectScope = getRawSelectScopeNonNull(select);
List<SqlNode> expandList = new ArrayList<>();
for (SqlNode orderItem : orderList) {
- SqlNode expandedOrderItem = expand(orderItem, orderScope);
+ SqlNode expandedCommon =
+ expandCommonColumnInOrderBy(orderItem, select, selectScope);
+ final SqlNode expandedOrderItem;
+ if (expandedCommon != orderItem) {
+ expandedOrderItem = expand(expandedCommon, orderScope);
Review Comment:
Here should not a double expansion: the two steps are complementary.
expandCommonColumnInOrderBy only substitutes an unqualified common column with
a qualified reference (or a `COALESCE` call for `FULL JOIN`), including inside
`DESC`/`NULLS` wrappers; it does no name resolution.
The regular expand(node, orderScope) that follows performs the actual
expansion (fullyQualify, alias/ordinal resolution, parenthesized-function
calls). The substitution result still needs to go through expand to be usable
by validateOrderItem and type derivation.
I've clarified this in the comment above the loop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
