Skip to content

[CALCITE-3495] RelDecorrelator generate plan with different semantics when handle Aggregate#1670

Closed
yanlin-Lynn wants to merge 1 commit intoapache:masterfrom
yanlin-Lynn:decorrelate-count
Closed

[CALCITE-3495] RelDecorrelator generate plan with different semantics when handle Aggregate#1670
yanlin-Lynn wants to merge 1 commit intoapache:masterfrom
yanlin-Lynn:decorrelate-count

Conversation

@yanlin-Lynn
Copy link
Contributor

In some case, the semantics of sql is changed after decorrelated, see CALCIT-3495 for detail.

This PR tries to fail the decorrelation for this kind of sql.

if (aggFunctionNotFitToDecorrelate(call.getAggregation())) {
return null;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we allow count agg for subquery where the group key is not empty? e.g.
select * from foo where (a, b) in (select c, count(*) from bar where bar.d=foo.d group by c)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I am not sure Calcite can decorrelate it or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than failing the decorrelation when COUNT, do we consider just disable decorrelation when subquery is an Aggregate?
Because user have the flexibility to define UDAF, it's hard to predicate the Agg behavior or enumerate all types of Agg functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yanlin-Lynn
If the PR is already for review, could you please remove [WIP] from PR title ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hsyuan , yes, we can allow that when group key is not empty. I add a test case for that.

@jinxing64 RelDecorrelator.decorrelateQuery is a public method, user may call it directly. So, I would perfer to fail the decorrelation. But, your concern is valid, so I add the check for UDAF. How about that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yanlin-Lynn
I would hear committers' advice.

@yanlin-Lynn yanlin-Lynn changed the title [WIP][CALCITE-3495] RelDecorrelator generate plan with different semantics when handle Aggregate [CALCITE-3495] RelDecorrelator generate plan with different semantics when handle Aggregate Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants