xiangfu0 commented on code in PR #20091:
URL: https://github.com/apache/pinot/pull/20091#discussion_r4414983090
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/validate/Validator.java:
##########
@@ -62,11 +71,108 @@ public boolean isNonStrictGroupBy() {
}
};
- public Validator(SqlOperatorTable opTab, SqlValidatorCatalogReader
catalogReader, RelDataTypeFactory typeFactory) {
+ private final boolean _preferSelectAliasInGroupBy;
+ /// `GROUP BY` names that match a `SELECT` alias, per `SELECT`. See
[#rewriteGroupByAliases].
+ private final Map<SqlSelect, List<String>> _groupByAliasNames = new
IdentityHashMap<>();
+ /// Whether [BrokerMeter#QUERIES_WITH_GROUP_BY_ALIAS_SHADOWED_BY_COLUMN] was
already marked for this query.
+ private boolean _groupByAliasShadowedByColumnFound;
+
+ /// @param preferSelectAliasInGroupBy whether a simple name in `GROUP BY`
resolves to a `SELECT` alias before a
+ /// column of the same name. See
[#rewriteGroupByAliases].
+ public Validator(SqlOperatorTable opTab, SqlValidatorCatalogReader
catalogReader, RelDataTypeFactory typeFactory,
+ boolean preferSelectAliasInGroupBy) {
super(opTab, catalogReader, typeFactory,
Config.DEFAULT.withConformance(PINOT_CONFORMANCE)
.withIdentifierExpansion(true)
.withTypeCoercionFactory(PinotTypeCoercion::new));
+ _preferSelectAliasInGroupBy = preferSelectAliasInGroupBy;
+ }
+
+ @Override
+ protected @PolyNull SqlNode performUnconditionalRewrites(@PolyNull SqlNode
node, boolean underFrom) {
+ SqlNode rewritten = super.performUnconditionalRewrites(node, underFrom);
+ if (rewritten instanceof SqlSelect) {
+ rewriteGroupByAliases((SqlSelect) rewritten);
+ }
+ return rewritten;
+ }
+
+ /// Finds each simple name in the `GROUP BY` list that matches exactly one
`SELECT` alias, unless the `SELECT` item is
+ /// the column of the same name. When [#_preferSelectAliasInGroupBy] is set,
replaces the name with the ordinal of
+ /// that `SELECT` item.
+ ///
+ /// Calcite 1.42 (CALCITE-7052) resolves a `GROUP BY` name to a column of
the `FROM` clause first, and to a `SELECT`
+ /// alias only when no column (or more than one) has that name. Calcite 1.40
resolved the alias first. Given
+ /// `SELECT CASE WHEN c = 'a' THEN 'x' ELSE c END AS c, COUNT(*) FROM t
GROUP BY c`, Calcite 1.40 groups by the
+ /// `CASE` expression and Calcite 1.42 groups by the column `c`. An ordinal
always refers to the `SELECT` item, so
+ /// the rewrite restores the Calcite 1.40 result. It also makes `HAVING`
resolve the name to the alias, as before.
+ ///
+ /// The names found are kept in [#_groupByAliasNames].
[#validateGroupClause] later checks which of them are also
+ /// columns. That check needs the scopes, which do not exist yet at this
point.
+ ///
+ /// Only top-level `GROUP BY` items are considered. Names inside `ROLLUP`,
`CUBE` and `GROUPING SETS` keep the
+ /// Calcite 1.42 resolution.
+ private void rewriteGroupByAliases(SqlSelect select) {
+ SqlNodeList groupList = select.getGroup();
+ if (groupList == null) {
+ return;
+ }
+ SqlNameMatcher nameMatcher = getCatalogReader().nameMatcher();
+ List<SqlNode> selectList = select.getSelectList().getList();
+ List<String> aliasNames = new ArrayList<>();
+ for (int i = 0; i < groupList.size(); i++) {
+ SqlNode groupItem = groupList.get(i);
+ if (!(groupItem instanceof SqlIdentifier) || !((SqlIdentifier)
groupItem).isSimple()) {
Review Comment:
Non-blocking. On the commit before the upgrade (Calcite 1.40), `SELECT
UPPER(col1) AS col1, COUNT(*) FROM a GROUP BY ROLLUP(col1)` grouped by
`UPPER(col1)`. On this head it groups by the raw column even with the option
on. Results change with no error, and the meter does not count the query
either. Calcite 1.42 already expands ordinals inside ROLLUP/CUBE/GROUPING SETS
(`GROUP BY ROLLUP(1)` plans correctly here), so the same rewrite could recurse
into those operands, including ROW. If you keep this out of scope, counting
these queries in the meter would at least let operators find them.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
