[CALCITE-6893] Remove agg from Union children in IntersectToDistinctRule #4246
Conversation
| @@ -44,25 +48,16 @@ | |||
| * | |||
| * <h2>Example</h2> | |||
| * | |||
| * <p>Query: <code>R1 Intersect All R2</code> | |||
| * <p>Query: <code>R1 Intersect R2</code> | |||
There was a problem hiding this comment.
Can you revise this whole section? The previous author used terms such as 'GB' (which is used in Hive but not in Calcite). I suspect that SQL queries are clearer than pseudo-relational algebra.
| // Generate count(*) filter (where i = n) as count_i{n} | ||
| aggCalls[i] = relBuilder.countStar(null) | ||
| .filter( | ||
| relBuilder.equals(relBuilder.field(indexAlias), |
There was a problem hiding this comment.
don't use indexAlias (a string); use an int
| aggCalls[i] = relBuilder.countStar(null) | ||
| .filter( | ||
| relBuilder.equals(relBuilder.field(indexAlias), | ||
| relBuilder.literal(i))).as(countPrefix + i); |
| } | ||
|
|
||
| // create a union above all the branches | ||
| final int branchCount = intersect.getInputs().size(); |
There was a problem hiding this comment.
i prefer the old name, branchCount, to inputCount.
| relBuilder.equals(relBuilder.field(fieldCount - 1), | ||
| rexBuilder.makeBigintLiteral(new BigDecimal(branchCount)))); | ||
| // Add filter count_i{n} > 0 for each branch | ||
| relBuilder.filter(relBuilder.and(filters)); |
There was a problem hiding this comment.
relBuilder.filter(relBuilder.and(filters)) → relBuilder.filter(filters)
|
|
||
| // Project all but the last field | ||
| relBuilder.project(Util.skipLast(relBuilder.fields())); | ||
| // Project all but the last added field(e.g. i and count_i{n}) |
| final int oriFieldCount = intersect.getRowType().getFieldCount(); | ||
| final int inputCount = intersect.getInputs().size(); | ||
|
|
||
| AggCall[] aggCalls = new AggCall[inputCount]; |
There was a problem hiding this comment.
use list rather than array
| relBuilder.push(input); | ||
| relBuilder.aggregate(relBuilder.groupKey(relBuilder.fields()), | ||
| relBuilder.countStar(null)); | ||
| final String countPrefix = "count_i"; |
There was a problem hiding this comment.
inline countPrefix. no need for a variable (when you start using field ordinals rather than field names)
| relBuilder.push(input); | ||
| relBuilder.aggregate(relBuilder.groupKey(relBuilder.fields()), | ||
| relBuilder.countStar(null)); | ||
| final String countPrefix = "count_i"; |
There was a problem hiding this comment.
You've removed more comments than you've added. This method doesn't need line-by-line comments but a 'here's the overall plan' comment is necessary. Give a good example of the SQL you intend to generate.
| // Add a constant column "i" for each input | ||
| fields.add( | ||
| relBuilder.alias( | ||
| rexBuilder.makeBigintLiteral(new BigDecimal(i)), indexAlias)); |
Jira link: CALCITE-6893