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

HTTP-83 Support SQL queries with project pushdown after a lookup join #84

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

OlivierZembri
Copy link
Contributor

Description

Ability to support SQL queries that add or remove columns on the result of a lookup join query.

Resolves #83

PR Checklist

@OlivierZembri OlivierZembri force-pushed the issue-83 branch 2 times, most recently from 05c7db6 to 1055ff1 Compare March 28, 2024 19:17
@OlivierZembri
Copy link
Contributor Author

@kristoffSC Could you please have look ?

@davidradl
Copy link
Contributor

@OlivierZembri Looks good to me. A couple of comments:
When you say "Added support for using the result of a lookup join operation in a subsequent select query that adds
or removes columns (project pushdown operation)." I was a bit confused by this wording, are we saying " Adding support for projection pushdown, so for lookup joins the projection columns will be honoured. Prior to this fix the projection columns were not honoured, so the output columns (the projections) could not be changed."

In terms of migration considerations for existing users, would the select be failed or ignored if a projection was attempted. If the projection was ignored then there maybe more or less columns returned to the user after the fix; which would be worth documenting as a side effect / migration consideration.

Copy link
Contributor

@AdrianVasiliu AdrianVasiliu left a comment

Choose a reason for hiding this comment

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

LGTM, especially in the light of the unit tests and the real-life tests done on our side.

@OlivierZembri
Copy link
Contributor Author

@davidradl

In terms of migration considerations for existing users, would the select be failed or ignored if a projection was attempted.

This is an enhancement proposal without which the Flink job fails with an exception
java.lang.UnsupportedOperationException: No implementation provided for SupportsProjectionPushDown. Please implement SupportsProjectionPushDown#applyProjection(int[][], DataType)

There might be some other usage this code change allow to support, but ours is defined in the description for this PR
(ie. #83)

@davidradl
Copy link
Contributor

@davidradl

In terms of migration considerations for existing users, would the select be failed or ignored if a projection was attempted.

This is an enhancement proposal without which the Flink job fails with an exception java.lang.UnsupportedOperationException: No implementation provided for SupportsProjectionPushDown. Please implement SupportsProjectionPushDown#applyProjection(int[][], DataType)

There might be some other usage this code change allow to support, but ours is defined in the description for this PR (ie. #83thanks for the confirmation @OlivierZembri . @kristoffSC unless you have any objections LGTM

@AdrianVasiliu
Copy link
Contributor

@kristoffSC It would make our life much easier if this PR could be reviewed by maintainers and merged relatively quickly.

There's also @davidradl 's #87 that we'll also need, but if reviewing/merging it will take more time, would it be possible to release the connector after the merge of the present PR, as this is the first blocker for our current work.

@grzegorz8
Copy link
Member

@kristoffSC It would make our life much easier if this PR could be reviewed by maintainers and merged relatively quickly.

There's also @davidradl 's #87 that we'll also need, but if reviewing/merging it will take more time, would it be possible to release the connector after the merge of the present PR, as this is the first blocker for our current work.

Both PRs approved with a few minor comments. I'll merge them once they are addressed.

Signed-off-by: David Radley <david_radley@uk.ibm.com>
@grzegorz8 grzegorz8 merged commit 3c57b39 into getindata:main Apr 3, 2024
3 checks passed
@OlivierZembri
Copy link
Contributor Author

@grzegorz8 Thanks for merging.
It would be great to have a release 0.13.0 so that we can use these changes.

@grzegorz8
Copy link
Member

@grzegorz8 Thanks for merging. It would be great to have a release 0.13.0 so that we can use these changes.

Release 0.13.0 build started: https://github.com/getindata/flink-http-connector/actions/runs/8544953258/job/23412174122

But I forgot to ask if we need to update any section in README.md, especially when it comes to #87

@AdrianVasiliu
Copy link
Contributor

@grzegorz8 For some reason https://github.com/getindata/flink-http-connector/actions/runs/8544953258/job/23412174122 failed. Could you please look into it?

Regarding the README.md: well, before the last PRs, it didn't cover the details of the extension points, as such I don't see a particular reason to need it covered in the readme now more than before ;-) But yes, of course, in the long run, it would be nice to have a more complete doc. Moreover, maybe one day a default out-of-the-box implementation will provide enough capabilities for handling common use-cases without the need to implement extension points. But this is a matter for future work.

@kristoffSC
Copy link
Collaborator

@AdrianVasiliu
Copy link
Contributor

@kristoffSC Yes, it did, thanks!

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.

Support SQL queries with project pushdown after a lookup join
5 participants