-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support aliases in PARTITION BY and GROUP BY #4952
Support aliases in PARTITION BY and GROUP BY #4952
Conversation
Conflicting files ksqldb-engine/src/main/java/io/confluent/ksql/analyzer/Analyzer.java ksqldb-engine/src/main/java/io/confluent/ksql/structured/SchemaKStream.java ksqldb-engine/src/test/java/io/confluent/ksql/structured/SchemaKStreamTest.java
Conflicting files ksqldb-engine/src/main/java/io/confluent/ksql/engine/rewrite/AstSanitizer.java ksqldb-engine/src/main/java/io/confluent/ksql/planner/LogicalPlanner.java ksqldb-streams/src/main/java/io/confluent/ksql/execution/streams/GroupByParamsFactory.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I did a pretty quick scan as it's a large PR but it seems pretty striaghtforward. If there's anything specific you want me to look at let me know
Let's not forget the docs PR after this :)
.getColumnName(); | ||
|
||
if (node.getAlias().get().equals(groupByColName)) { | ||
// Alias is a no-op - remove it: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wondering, why not just go through with it?
Docs will be covered by: #4686 in a single pass. |
Description
fixes: #4881
fixes: #4813
The commit adds support for aliases in GROUP BY and PARTITION BY clauses. For example:
This is particularly useful where the new key column name would be generated, e.g.
PARTITION BY only supports a single value expression, so aliasing on this is as you'd expect
PARTITION BY X as Y
.GROUP BY supports multiple value expressions, e.g.
GROUP BY X, Y
. This will currently create a new STRING KEY with a generated name e.g.KSQL_COL_0
. The commit allows the full set of columns to be aliased, e.g.GROUP BY (X, Y) AS Z
. This is an interim solution. Once ksqlDB supports multiple key columns, the ability to alias the set of columns will be removed, and instead support will be added to alias each expression in the GROUP BY e.g.GROUP BY W as X, Y as Z
.Testing done
usual
How to review.
Review the first commit which:
PartitionBy
much like the existingGroupBy
AstNode
.Review the second commit which wires in the aliases. Flow is:
GroupBy
andPartitionBy
.SqlFormatter
to know about aliases.Analysis
to take the wholeGroupBy
andPartitionBy
nodes, rather than extract info out of them.LogicalPlanner
to use the alias when building schemas and to pass it along where needed.StreamsSelectKey
query plan step, used for PARTITION BY, to have an optional alias. (Backwards compatible as its optional)StreamsGroupBy
andTableGroupBy
query plan steps, used by GROUP BY, to have an optional alias. (Backwards compatible as its optional)Reviewer checklist