SQL: NPE when using the same alias for a projection and an aggregate and GROUPed BY

[astefan]

SELECT gender AS g, SUM(salary) AS g FROM test_emp GROUP BY g

Notice the alias name (which is the same) which, used in the GROUP BY will trigger a NPE:

[elasticsearch] java.lang.NullPointerException: null
[elasticsearch]         at org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer$ResolveOrdinalInOrderByAndGroupBy.findOrdinal(Analyzer.java:593) ~[?:?]
[elasticsearch]         at org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer$ResolveOrdinalInOrderByAndGroupBy.rule(Analyzer.java:558) ~[?:?]
[elasticsearch]         at org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer$AnalyzeRule.lambda$apply$0(Analyzer.java:1287) ~[?:?]
[elasticsearch]         at org.elasticsearch.xpack.sql.tree.Node.lambda$transformUp$11(Node.java:196) ~[?:?]
[elasticsearch]         at org.elasticsearch.xpack.sql.tree.Node.transformUp(Node.java:190) ~[?:?]
[elasticsearch]         at org.elasticsearch.xpack.sql.tree.Node.transformUp(Node.java:196) ~[?:?]
[elasticsearch]         at org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer$AnalyzeRule.apply(Analyzer.java:1287) ~[?:?]
[elasticsearch]         at org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer$AnalyzeRule.apply(Analyzer.java:1281) ~[?:?]
[elasticsearch]         at org.elasticsearch.xpack.sql.rule.RuleExecutor$Transformation.<init>(RuleExecutor.java:82) ~[?:?]
[elasticsearch]         at org.elasticsearch.xpack.sql.rule.RuleExecutor.executeWithInfo(RuleExecutor.java:158) ~[?:?]
[elasticsearch]         at org.elasticsearch.xpack.sql.rule.RuleExecutor.execute(RuleExecutor.java:130) ~[?:?]
[elasticsearch]         at org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer.analyze(Analyzer.java:132) ~[?:?]
[elasticsearch]         at org.elasticsearch.xpack.sql.session.SqlSession.lambda$analyzedPlan$0(SqlSession.java:110) ~[?:?]
[elasticsearch]         at org.elasticsearch.xpack.sql.session.SqlSession.lambda$preAnalyze$2(SqlSession.java:144) ~[?:?]
[elasticsearch]         at org.elasticsearch.action.ActionListener$1.onResponse(ActionListener.java:63) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[elasticsearch]         at org.elasticsearch.xpack.sql.analysis.index.IndexResolver.lambda$resolveAsMergedMapping$3(IndexResolver.java:277) [x-pack-sql-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[elasticsearch]         at org.elasticsearch.action.ActionListener$1.onResponse(ActionListener.java:63) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[elasticsearch]         at org.elasticsearch.client.node.NodeClient.lambda$executeLocally$0(NodeClient.java:89) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[elasticsearch]         at org.elasticsearch.tasks.TaskManager$1.onResponse(TaskManager.java:144) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[elasticsearch]         at org.elasticsearch.tasks.TaskManager$1.onResponse(TaskManager.java:138) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[elasticsearch]         at org.elasticsearch.action.fieldcaps.TransportFieldCapabilitiesAction.lambda$doExecute$0(TransportFieldCapabilitiesAction.java:88) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[elasticsearch]         at org.elasticsearch.action.fieldcaps.TransportFieldCapabilitiesAction$1.onResponse(TransportFieldCapabilitiesAction.java:101) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
[elasticsearch]         at org.elasticsearch.action.fieldcaps.TransportFieldCapabilitiesAction$1.onResponse(TransportFieldCapabilitiesAction.java:97) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]

[elasticmachine]

Pinging @elastic/es-search

[verkhovin]

@astefan can i take a look at this issue?
It seems that the query isn't correct. So what is expected behavior for the case?

[matriv]

@verkhovin Please feel free to take a look.
The expected behavior would be to throw an Exception ideally from the Verifier with an error message like this:

1:61 GROUP BY "g" is ambiguous

[verkhovin]

@matriv I'll write here if I find something interesting. Thank you.

[verkhovin]

Here what I've found.
The error is caused by org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer.ResolveRefs resolver. It calls resolveAgainstList(...) method (refer to Analyzer.java line 340), which tries to resolve alias from group by statement against aggregates resolved before. In the given case it obviously fails (as there is 2 matches between aggregates and alias) and returns UnresolvedAttribute (refer to Analyzer.java line 198) instance.
Then this instance is used to get corresponding aggregate from resolved map (refer to Analyzer.java line 344), here null is returned from the map. And in lines 347-350 this null value is passed to newly constructed aggregate.

The NPE is thrown on the next resolver (ResolveOrdinalInOrderByAndGroupBy), as Aggregate returned from ResolveRefs contains null value in groupings.

@matriv @astefan Can you please suggest how should I fix it in a proper way.
It seems that Verifier works after Analyzer, so we should handle described situation in Analyzer somehow without throwing exception and then fail when will Verifier start work.

[matriv]

@elastic/es-ql

[matriv]

Hi @verkhovin,

Apologies for stalling this issue, there has been some important refactoring meanwhile in #49693
that could change the approach of catching the duplicate aliases. Would you have time to work again on this? We would be happy to provide guidance if so.

[verkhovin]

@matriv Hi
sorry, i missed the notification. If it is still relevant, i can get it in the nearest future

[matriv]

@verkhovin Yes, it's still relevant.