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

PHOENIX-4871: ParameterMetadata for join query plans is empty #511

Merged
merged 1 commit into from
Sep 4, 2019

Conversation

BoxMiles
Copy link
Contributor

No description provided.

@gjacoby126
Copy link
Contributor

@twdsilva @ChinmaySKulkarni - Mind taking a look? Patch looks good to me but you guys know the read path better than I do.

Copy link
Contributor

@gjacoby126 gjacoby126 left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

@ChinmaySKulkarni ChinmaySKulkarni left a comment

Choose a reason for hiding this comment

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

+1. Can you also add the test case that you mentioned on the JIRA (to make sure that this does not surface) and perhaps add a comment there pointing to this JIRA. Otherwise, lgtm!

@mspielberg
Copy link

The tests already added are a minimized version of the misbehaving query in the JIRA, just with the names changed to align with the existing schema in the test suites. I can add a comment to those two test cases pointing them back to the JIRA. I can also add an integration test to the phoenix-query-server repo for end-to-end, but that would have to be a different PR in the other repository.

@gjacoby126
Copy link
Contributor

@BoxMiles , could you please rebase against master? Looks like there's a merge conflict that prevents this patch from being committed.

change constructor parameter order
add upserts to QueryServerBasicsIT
pass down BindManager to child QueryCompilers created for explicit subqueries
@mspielberg
Copy link

@gjacoby126 Do you need additional action on this?

@gjacoby126 gjacoby126 merged commit c6db8d5 into apache:master Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants