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

#1938 GEO_DISTANCE function broken by new UDF Invoker #2700

Merged
merged 9 commits into from May 8, 2019

Conversation

@uurl
Copy link
Contributor

commented Apr 17, 2019

Description

This PR fixes #1938 GEO_DISTANCE function broken by new UDF Invoker

Testing done

mvn clean package

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 #")

@uurl uurl requested a review from confluentinc/ksql as a code owner Apr 17, 2019

@agavra
Copy link
Contributor

left a comment

Thanks @uurl! I think the preferred approach, however, would be to move this to the new UDF framework (e.g. using the @UdfDescription annotation, you can see an example in DateToString.java). This way, it will require primitives (non-null) to match the function without the explicit null checks simply by looking at the signature of the method and seeing primitives as parameters.

I would also recommend that you add a test for this using our QueryTranslationTest framework, which you can see an example in datestring.json, including a case for when it should _not _work (e.g. nulls are passed in). To see an example syntax for expected exception take a look at cast.json

@agavra agavra requested a review from confluentinc/ksql Apr 17, 2019

@big-andy-coates big-andy-coates requested review from rodesai, hjafarpour and vcrfxia and removed request for confluentinc/ksql May 1, 2019

@vcrfxia vcrfxia requested a review from agavra May 1, 2019

@agavra
Copy link
Contributor

left a comment

This is looking really good! Just a few comments in-line :)

@agavra agavra self-requested a review May 1, 2019

@agavra
Copy link
Contributor

left a comment

Thanks @uurl! Some more comments in-line, also please check why the tests are failing https://jenkins.confluent.io/job/confluentinc-pr/job/ksql/job/PR-2700/12/testReport/ - I think the comments that I have given will address them, but you can run the tests locally with mvn clean install

@agavra agavra self-requested a review May 2, 2019

@agavra

agavra approved these changes May 3, 2019

Copy link
Contributor

left a comment

LGTM! Thanks for the contribution @uurl - we may need to rebase to get the tests passing, the current failure seems unrelated to your change. I'll ask some folk to give you a second review and then we can get this merged.

@uurl

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@vcrfxia
Copy link
Contributor

left a comment

Thanks for the contribution, @uurl ! Apologies I've left so many nits, but they shouldn't take long to address and I'm happy to get this merged for you as soon as that happens.

@uurl

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

@vcrfxia Ready, thanks

@vcrfxia

vcrfxia approved these changes May 8, 2019

Copy link
Contributor

left a comment

Thanks so much @uurl -- merging now!

@vcrfxia vcrfxia merged commit 5914735 into confluentinc:master May 8, 2019

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@uurl uurl deleted the uurl:issue1938 branch May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.