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

fix: query w/ scoped all columns, join and where clause #5684

Merged

Conversation

big-andy-coates
Copy link
Contributor

@big-andy-coates big-andy-coates commented Jun 25, 2020

Description

fixes: #5503

This fixes a regression introduced in 0.10.

The implications of this are not that it crashes the command runner thread, as the original ticket states. Instead, the implications are that queries a users could previously run will now fail. Existing running queries will not be affected.

Affected queries will be any using combining:

  • A join
  • with a projection using a scoped 'all columns', e.g. a A.*
  • with a where clause

e.g.

SELECT A.*, B.Id
   FROM A
     JOIN B ON A.Id = B.userId
   WHERE A.x < 10;

Such queries now work.

Testing done

Usual

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

confluentinc#5503

This fixes a regression introduced in 0.10.

The implications of this are not that it crashes the command runner thread, as the original ticket states. Instead, the implications are that queries a users could previously run will now fail. Existing running queries will not be affected.

Affected queries will be any using combining:

* A join
* with a projection using a scoped 'all columns', e.g. a A.*
* with a where clause

e.g.

```
SELECT A.*, B.Id
   FROM A
     JOIN B ON A.Id = B.userId
   WHERE A.x < 10;
```
@big-andy-coates big-andy-coates requested a review from a team as a code owner June 25, 2020 15:44
@big-andy-coates big-andy-coates added this to the 0.11.0 milestone Jun 25, 2020
@big-andy-coates big-andy-coates changed the base branch from master to 6.0.x June 25, 2020 15:48
Comment on lines +106 to +108
.filter(s -> !sourceName.isPresent()
|| !s.getSourceName().isPresent()
|| sourceName.equals(s.getSourceName()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To understand the solution, first understand the logical model of the query:

Source A            Source B
   |                   |
   --------Join---------
             |
       Where Filter
             |
       Final Projection

The final SELECT / projection node calls its parent to resolve any select stars. In the case of A.* it passes A as the source name to expand when calling resolveSelectStart.

The parent WHERE filter uses the default impl of the resolveSelectStar method, which just looks for a parent node with a matching source name, if one is provided.

A Join node does not have a source name, i.e. it's neither A or B, so the default resolveSelectStart was ignoring it.

The fix is to change the default method to include any parent node that has no source name, thereby including the parent join node.

@big-andy-coates big-andy-coates modified the milestones: 0.11.0, 0.10.1 Jun 29, 2020
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

LGTM! Nice catch and thanks for the inline description @big-andy-coates

@big-andy-coates big-andy-coates merged commit 304eb0c into confluentinc:6.0.x Jun 30, 2020
@big-andy-coates big-andy-coates deleted the scoped_all_columns_bug branch June 30, 2020 11:16
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.

None yet

2 participants