Skip to content

Conversation

takezoe
Copy link
Member

@takezoe takezoe commented Dec 28, 2022

My previous pull request (#2674) caused unresolved grouping keys in some cases.

I think resolveSortItemIndexes may need the same fix, but not 100% sure.

@github-actions github-actions bot added the bug label Dec 28, 2022
case Alias(_, _, expr, _) =>
expr
case other =>
other
Copy link
Member Author

@takezoe takezoe Dec 28, 2022

Choose a reason for hiding this comment

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

Reverted #2674 but added unwrapping Alias to avoid wrong SQL generation in GROUP BY clause.

@@ -364,8 +367,6 @@ object TypeResolver extends LogSupport {
r.sourceColumn
case a: Alias =>
findSourceColumn(a.expr)
case s: SingleColumn =>
findSourceColumn(s.expr)
Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted #2674

@codecov
Copy link

codecov bot commented Dec 28, 2022

Codecov Report

Merging #2680 (b90263a) into master (9a387b7) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2680      +/-   ##
==========================================
- Coverage   82.22%   82.20%   -0.02%     
==========================================
  Files         337      337              
  Lines       14074    14065       -9     
  Branches     2186     2196      +10     
==========================================
- Hits        11572    11562      -10     
- Misses       2502     2503       +1     
Impacted Files Coverage Δ
...ala/wvlet/airframe/sql/analyzer/TypeResolver.scala 91.89% <66.66%> (-0.68%) ⬇️
...n/scala/wvlet/airframe/sql/model/LogicalPlan.scala 88.84% <0.00%> (-0.39%) ⬇️
...me-log/src/main/scala/wvlet/log/io/StopWatch.scala 81.05% <0.00%> (+0.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a387b7...b90263a. Read the comment docs.

@takezoe takezoe force-pushed the fix-resolveAggregationIndexes-2 branch from be9627e to b90263a Compare December 28, 2022 13:41
@xerial xerial self-requested a review December 28, 2022 22:13
@takezoe takezoe merged commit ea52d08 into wvlet:master Dec 28, 2022
@takezoe takezoe deleted the fix-resolveAggregationIndexes-2 branch December 28, 2022 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants