mihaibudiu commented on code in PR #4548:
URL: https://github.com/apache/calcite/pull/4548#discussion_r2365857527
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -487,6 +487,14 @@ private boolean expandSelectItem(final SqlNode selectItem,
SqlSelect select,
// calls.
selectScope = getSelectScope(select);
expanded = expandSelectExpr(selectItem, scope, select, expansions);
+
+ // Non-strict GROUP BY: wrap non-aggregated, non-grouped columns in
ANY_VALUE()
+ if (isAggregate(select)
+ && config.conformance().isNonStrictGroupBy()
+ && isNonAggregatedNonGroupedColumn(expanded, select)) {
Review Comment:
I wonder how this interacts with other config options such as isGroupByAlias
or isSelectAlias.
But perhaps this is fine, since this check is after expandSelectExpr.
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -526,6 +534,45 @@ private boolean expandSelectItem(final SqlNode selectItem,
SqlSelect select,
return false;
}
+ /**
+ * Returns true if the node is a non-aggregated, non-grouped column in
SELECT.
+ */
+ private boolean isNonAggregatedNonGroupedColumn(SqlNode node, SqlSelect
select) {
+ if (aggFinder.findAgg(node) != null) {
+ return false;
+ }
+
+ if (node instanceof SqlIdentifier) {
+ SqlNodeList groupList = select.getGroup();
+ if (groupList == null) {
+ return true;
+ }
+ return groupList.getList().stream()
+ .noneMatch(groupItem -> groupItem != null
+ && equalAsIdentifier((SqlIdentifier) node, groupItem));
+ }
+
+ if (node instanceof SqlCall) {
+ return ((SqlCall) node).getOperandList().stream()
+ .allMatch(operand -> isNonAggregatedNonGroupedColumn(operand,
select));
+ } else if (node instanceof SqlLiteral) {
+ return true;
+ }
+
+ return false;
+ }
+
+ /** Determines whether two SqlIdentifier objects are equivalent. */
+ private boolean equalAsIdentifier(SqlIdentifier id1, SqlNode node2) {
+ if (!(node2 instanceof SqlIdentifier)) {
Review Comment:
This function needs many more tests.
Ideally you would not compare identifiers, but column indexes in inputs.
Are you sure that the compared identifiers are both fully expanded at this
point?
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java:
##########
@@ -130,6 +130,12 @@ enum SelectAliasLookup {
*/
SelectAliasLookup isSelectAlias();
+ /**
+ * Allow non-GROUP BY columns in SELECT (MySQL loose mode).
+ * If enabled, wrap with ANY_VALUE().
Review Comment:
I don't think that SqlConformance should dictate how this is implemented.
The second line should not be here.
--
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]
