Skip to content

Add support for Union All#6351

Merged
harshit-gangal merged 13 commits intovitessio:masterfrom
planetscale:union-all
Jul 1, 2020
Merged

Add support for Union All#6351
harshit-gangal merged 13 commits intovitessio:masterfrom
planetscale:union-all

Conversation

@harshit-gangal
Copy link
Copy Markdown
Member

@harshit-gangal harshit-gangal commented Jun 22, 2020

Added supported for union all. At VTGate in memory concatenation of results will happen.
Current Limitation: If there is column type mismatch between different union it will fail than resolving the type.

Closes: #4613

@harshit-gangal harshit-gangal requested a review from sougou June 22, 2020 12:21
@harshit-gangal harshit-gangal marked this pull request as draft June 22, 2020 12:21
@systay systay force-pushed the union-all branch 3 times, most recently from ffade1a to 6c5a535 Compare June 23, 2020 14:44
@harshit-gangal harshit-gangal marked this pull request as ready for review June 23, 2020 16:51
@systay systay force-pushed the union-all branch 2 times, most recently from cc061e4 to de47c6a Compare June 24, 2020 09:28
Copy link
Copy Markdown
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

In case you haven't, can you also make sure that you've sanity checked code coverage?

@harshit-gangal harshit-gangal force-pushed the union-all branch 2 times, most recently from be41b8c to 19d0d98 Compare June 26, 2020 17:50
@harshit-gangal harshit-gangal requested a review from sougou June 26, 2020 17:52
@harshit-gangal harshit-gangal force-pushed the union-all branch 4 times, most recently from 6406ec1 to d6e5d51 Compare June 29, 2020 03:26
Copy link
Copy Markdown
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

Minor nits.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As a general rule, we should not change the caller's context. Although things will work as expected in this case, it's better to still follow the rule. This means that you have to create a local cancel-able one and use that instead.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we need the engine primitives to use this new cancelable context. Contexts are like onions - there are layers to them. We are merely adding a cancelable layer to the context the caller provided us with. if you want, we could peel that layer before leaving this method, to restore the original context.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh. I didn't realize that you were passing vcursor down to the callees. So, it's ok to leave it as is for now.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The context created here needs to be sent down to calls from here one. So that once the context is cancelled. The lower layers can also stop processing.
The issue here is the lower layer can receive this new context via vcursor as of now. So, the current way was to replace it.
Once we start passing context separately this would not be an issue.

@systay The current suggestion does not hold. Because if we replace the vcursor context and there is parent branch which has vcursor usage. It will already be updated and using the new context. So peeling off before returning may not guarantee things are in order.

@harshit-gangal harshit-gangal requested a review from sougou June 30, 2020 10:50
Copy link
Copy Markdown
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

You can merge after resolving conflicts.

harshit-gangal and others added 9 commits July 1, 2020 09:25
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
harshit-gangal and others added 4 commits July 1, 2020 09:26
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
@harshit-gangal harshit-gangal merged commit 354e3d0 into vitessio:master Jul 1, 2020
@deepthi deepthi added this to the v7.0 milestone Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve UNION support

4 participants