Clean expression stack before resolving project list #1059
Conversation
Signed-off-by: Lantao Jin <ltjin@amazon.com>
LantaoJin
requested review from
dai-chen,
mengweieric,
penghuo,
seankao-az,
anirudha,
kaituo,
YANG-DB,
noCharger and
ykmr1224
as code owners
| public void resetNamedParseExpressions() { | ||
| getNamedParseExpressions().retainAll(emptyList()); | ||
| } |
There was a problem hiding this comment.
Add a new reset method to replace context.retainAllNamedParseExpressions(e -> e);
context.retainAllNamedParseExpressions(e -> e); (reset expression stack) is remarkably prone to oversight, consequently leading to errors.
There was a problem hiding this comment.
Frequent clearing of the expression stack could potentially affect the overall query execution time, especially for large or complex queries. Can it happen?
There was a problem hiding this comment.
Frequent clearing of the expression stack could potentially affect the overall query execution time, especially for large or complex queries. Can it happen?
No. The overhead of stack management operations is statistically insignificant compared to core processing tasks.
| @@ -460,6 +464,9 @@ public LogicalPlan visitProject(Project node, CatalystPlanContext context) { | |||
| context.withProjectedFields(node.getProjectList()); | |||
| } | |||
| LogicalPlan child = visitFirstChild(node, context); | |||
|
|
|||
| // reset expression stack before resolving project | |||
| context.resetNamedParseExpressions(); | |||
There was a problem hiding this comment.
This reset calling in visitProject is the key fix.
| @@ -371,6 +373,8 @@ public LogicalPlan visitSubqueryAlias(SubqueryAlias node, CatalystPlanContext co | |||
| @Override | |||
| public LogicalPlan visitAggregation(Aggregation node, CatalystPlanContext context) { | |||
| visitFirstChild(node, context); | |||
| // clean before to go | |||
| context.resetNamedParseExpressions(); | |||
There was a problem hiding this comment.
Missing reset calling in visitAggregation as well. This is not the fix for "Fields command" issue. But after the "Fields command" fixed, below scalar subquery will throw MISSING_GROUP_BY exception. This line is to fix the potential problem.
source = $outerTable as o
| where id = [
source = $innerTable as i | where o.id = i.uid | stats max(i.uid)
]
| fields o.id, o.name
See the new tests I added in FlintSparkPPLScalarSubqueryITSuite
|
Could you help to review this? @YANG-DB @qianheng-aws |
|
Thanks for the quick fix |
|
@LantaoJin Please backport to 0.x if it is not a breaking change. We started similar branching as sql repository. |
Signed-off-by: Lantao Jin <ltjin@amazon.com> (cherry picked from commit 09aba20) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit 09aba20) Signed-off-by: Lantao Jin <ltjin@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
Issue described in #1058.
Root cause is missing reset expression stack before resolving project list.
Related Issues
Resolves #1058
Check List
--signoffBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.