ksql icon indicating copy to clipboard operation
ksql copied to clipboard

Handling of parameterized complex types in arguments to UDFs

Open anekdoti opened this issue 6 years ago • 10 comments

Fixes #2085.

Description

Formerly, it was not possible to load variants of the same UDF that only differ in the type parameter of the complex argument type, e.g., f(List<String> list) and f(List<Long> list). This was due to that the class FunctionParameter, which encapsulates an argument of a UDF, only stored the type attribute of the argument schema and not the whole schema, and thus ignoring the type parameters of Maps and Arrays.

The bugfix changes FunctionParameter to store the whole schema of the argument type and modifies its methods to take into account type parameters of Maps and Arrays.

Moreover, the toString method of KsqlFunction now uses SchemaUtil::getSqlTypeName to stringify argument types so that type parameters of Maps and Arrays are no longer omitted.

Testing done

All unit and integration tests of KSQL succeed. I tested the UdfFactory being able to distinguish between type parameters with a UDF in the manner described in #2085.

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

anekdoti avatar Oct 25 '18 07:10 anekdoti

It looks like @anekdoti hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence. Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

ghost avatar Oct 25 '18 07:10 ghost

It looks like @anekdoti hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence. Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

[clabot:check]

anekdoti avatar Oct 25 '18 07:10 anekdoti

@confluentinc It looks like @anekdoti just signed our Contributor License Agreement. :+1:

Always at your service,

clabot

ghost avatar Oct 25 '18 07:10 ghost

Hey @anekdoti, thanks for raising the PR!

Are you able to add some test cases to this please?

big-andy-coates avatar Oct 30 '18 01:10 big-andy-coates

Hello big-andy-coates,

thanks for taking a look at my PR! I just added two test cases which show that functions that differ in the type parameter of their complex argument type do not raise an exception anymore.

anekdoti avatar Oct 30 '18 13:10 anekdoti

Hello @dguy and @big-andy-coates ,

is there something I can do to bring my pull request forward?

All the best, anekdoti

anekdoti avatar Nov 20 '18 13:11 anekdoti

Hi @anekdoti, sorry about the delay. I think my previous comment still stands, i.e., I think we need some tests to prove that the correct functions are invoked when we have a UDF with parameterized functions.

dguy avatar Nov 20 '18 20:11 dguy

Hi @dguy,

thanks for your reply. Your comment makes it clearer which tests are still missing - I will do provide them in the following days.

anekdoti avatar Nov 21 '18 08:11 anekdoti

I believe this is no longer a problem after #2595, which introduced the new UdfIndex that takes into account the argument schema.

agavra avatar May 01 '19 16:05 agavra

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

:x: dguy
:x: anekdoti
You have signed the CLA already but the status is still pending? Let us recheck it.

cla-assistant[bot] avatar Nov 15 '23 20:11 cla-assistant[bot]