Skip to content
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

airframe-sql: Remove unnecessary condition from AllColumns resolution #3061

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

takezoe
Copy link
Member

@takezoe takezoe commented Jul 13, 2023

Follow up to #3057

@takezoe takezoe requested a review from xerial July 13, 2023 09:48
@takezoe takezoe marked this pull request as ready for review July 13, 2023 09:48
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #3061 (2483d9e) into master (ce646df) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3061      +/-   ##
==========================================
- Coverage   82.81%   82.80%   -0.02%     
==========================================
  Files         350      350              
  Lines       14666    14665       -1     
  Branches     2448     2505      +57     
==========================================
- Hits        12146    12143       -3     
- Misses       2520     2522       +2     
Impacted Files Coverage Δ
...ala/wvlet/airframe/sql/analyzer/TypeResolver.scala 93.64% <100.00%> (ø)

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@@ -463,7 +463,7 @@ object TypeResolver extends LogSupport {
case other => toResolvedAttribute(other.name, other)
}
List(a.copy(columns = Some((qualifier match {
case Some(q) => allColumns.filter(c => c.qualifier.contains(q) || c.tableAlias.contains(q))
case Some(q) => allColumns.filter(_.tableAlias.contains(q))
Copy link
Member

Choose a reason for hiding this comment

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

Good. This is because AllColumn.outputAttributes have both the qualifier and tableAlias

Copy link
Member Author

@takezoe takezoe Jul 14, 2023

Choose a reason for hiding this comment

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

This is because AllColumn.outputAttributes have both the qualifier and tableAlias

This is not accurate explanation.

Without this fix, we needed to check qualifiers for the following case but now this case is covered by tableAlias.

select t1.* from (select t1.time from A) t1

On the other hand, checking qualifiers allows to resolve cases like below, but actually these are not valid SQL which means the current logic can resolve even invalid cases.

select A.* from (select A.time from A)
select t1.* from (select t1.time from A t1)

@xerial xerial added the internal Internal changes (usually non-user facing) label Jul 13, 2023
@xerial xerial merged commit b5fd84b into wvlet:master Jul 13, 2023
@takezoe takezoe deleted the fix-resolve-aliased-all-columns branch July 14, 2023 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Internal changes (usually non-user facing)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants