Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/3248#discussion_r20393434
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
---
@@ -122,10 +115,15 @@ class Analyzer(catalog: Catalog, registry:
FunctionRegistry, caseSensitive: Bool
case e => e.children.forall(isValidAggregateExpression)
}
- aggregateExprs.foreach { e =>
- if (!isValidAggregateExpression(e)) {
- throw new TreeNodeException(plan, s"Expression not in GROUP
BY: $e")
- }
+ aggregateExprs.find { e =>
+ !isValidAggregateExpression(e.transform {
+ // Should trim aliases around `GetField`s. These aliases are
introduced while
+ // resolving struct field accesses, because `GetField` is
not a `NamedExpression`.
+ // (Should we just turn `GetField` into a `NamedExpression`?)
--- End diff --
An earlier version of catalyst actually did something similar to having
`GetField` be a named expression. However, this complicated a lot of things.
In particular, it made recursive schemas impossible and it is a little tricky
to have non-leaf nodes that have to have expression ids.
That said, I'm not opposed to this idea in general, but I think it might be
a fair amount of work to realize. We should possible evaluate a few options as
the current situation also seems less than ideal.
I'd propose we merge this now to fix the bug and investigate after 1.2.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
