Fix exception during plan duplicate columns in order by clause #3018
Conversation
| @@ -629,8 +630,15 @@ private PlanBuilder sort(PlanBuilder subPlan, List<SortItem> orderBy, Optional<S | |||
|
|
|||
| ImmutableList.Builder<Symbol> orderBySymbols = ImmutableList.builder(); | |||
| ImmutableMap.Builder<Symbol, SortOrder> orderings = ImmutableMap.builder(); | |||
| Set<Symbol> orderBySymbolSet = new HashSet<Symbol>(); | |||
There was a problem hiding this comment.
Instead of keeping track of the symbols, I'd change the code to build a HashMap for orderings instead of Guava's ImmutableMap.builder.
One thing to be careful with is that if the sort order for a given column is different, the first one should win. For instance, in:
SELECT a, a FROM t ORDER BY a ASC, a DESC
DESC for the second occurrence is effectively a no-op.
I'd add a test for this while you're at it.
|
@martint comments addressed. |
| for (FieldOrExpression fieldOrExpression : orderByExpressions) { | ||
| Symbol symbol = subPlan.translate(fieldOrExpression); | ||
| orderBySymbols.add(symbol); | ||
| if (orderings.containsKey(symbol)) { | ||
| sortItems.next(); |
There was a problem hiding this comment.
Instead of calling next() in the two branches (which makes the code a little more error prone), assign sortItems.next() to a variable before the if block and use it below.
Also, invert the if condition. I think the code will read more natural:
for (...) {
...
if (!orderings.contains(...)) {
... add to map, etc
}
}
|
@martint thanks, comments addressed. |
|
Merged, thanks! |
fix #1621
@martint any suggestion?