feat: change the public API of schema provider method #3287

vpapavas · 2019-08-29T21:11:45Z

Description

Changed the signature of Schema provideSchema(List<Schema>) to use SqlType instead.
I updated the docs as well.

Testing done

No new tests added, updated existing ones.

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

… Schema

ghost · 2019-08-29T21:11:47Z

@confluentinc It looks like @vpapavas just signed our Contributor License Agreement. 👍

Always at your service,

clabot

agavra

LGTM! Not sure I understand the dependency changes though

agavra · 2019-08-29T22:12:56Z

ksql-common/src/main/java/io/confluent/ksql/function/KsqlFunction.java

@@ -177,9 +178,11 @@ private void checkMatchingReturnTypes(final Schema s1, final Schema s2) {
    if (!SchemaUtil.areCompatible(s1, s2)) {
      throw new KsqlException(String.format("Return type %s of UDF %s does not match the declared "
                                                + "return type %s.",
-                                            s1.toString(),
+                                            SchemaConverters.connectToSqlConverter().toSqlType(


I think it may actually be preferable to use the SqlType#toString instead of converting it to Connect schema first! That way the user gets ARRAY<INTEGER> in sql syntax.

Yep, the new SqlTypes have a toString and toString(FormatOptions) and output correct SQL.

I am actually converting from Schema to SqlType so that the user gets the message in sql syntax

agavra · 2019-08-29T22:13:15Z

ksql-engine/pom.xml

@@ -167,6 +167,14 @@
            <artifactId>wiremock-jre8</artifactId>
            <scope>test</scope>
        </dependency>
+      <dependency>
+        <groupId>io.confluent.ksql</groupId>
+        <artifactId>ksql-streams</artifactId>


Don't think this is needed

agavra · 2019-08-29T22:13:37Z

ksql-streams/pom.xml

@@ -69,6 +69,10 @@
            <version>${project.version}</version>
            <scope>test</scope>
        </dependency>
+      <dependency>
+        <groupId>io.confluent.ksql</groupId>
+        <artifactId>ksql-common</artifactId>


agavra · 2019-08-29T22:14:38Z

ksql-engine/src/main/java/io/confluent/ksql/function/UdfLoader.java

-    return parameterTypes -> {
-      return invokeSchemaProviderMethod(instance, m, parameterTypes, annotation);
+    return parameterSchemas -> {
+      final List<SqlType> parameterTypes = parameterSchemas.stream().map(p -> SchemaConverters


nit: with multi-statement streams it's much easier to read on multiple lines

parameterSchemas.stream() .map(p -> ...) .collect(...)

JimGalasyn

LGTM!

big-andy-coates

LGTM, except for the dependency changes.

Thanks for making this change - I think it's worth it - much better public API and better error messages too!

big-andy-coates · 2019-09-03T15:57:18Z

ksql-common/src/main/java/io/confluent/ksql/function/KsqlFunction.java

@@ -177,9 +178,11 @@ private void checkMatchingReturnTypes(final Schema s1, final Schema s2) {
    if (!SchemaUtil.areCompatible(s1, s2)) {
      throw new KsqlException(String.format("Return type %s of UDF %s does not match the declared "
                                                + "return type %s.",
-                                            s1.toString(),
+                                            SchemaConverters.connectToSqlConverter().toSqlType(


Yep, the new SqlTypes have a toString and toString(FormatOptions) and output correct SQL.

big-andy-coates · 2019-09-03T15:57:45Z

ksql-engine/pom.xml

@@ -167,6 +167,14 @@
            <artifactId>wiremock-jre8</artifactId>
            <scope>test</scope>
        </dependency>
+      <dependency>
+        <groupId>io.confluent.ksql</groupId>
+        <artifactId>ksql-streams</artifactId>


Don't think this is needed

big-andy-coates · 2019-09-03T15:57:59Z

ksql-engine/src/main/java/io/confluent/ksql/function/UdfLoader.java

-    return parameterTypes -> {
-      return invokeSchemaProviderMethod(instance, m, parameterTypes, annotation);
+    return parameterSchemas -> {
+      final List<SqlType> parameterTypes = parameterSchemas.stream().map(p -> SchemaConverters


big-andy-coates · 2019-09-03T16:00:17Z

ksql-engine/src/main/java/io/confluent/ksql/function/udf/math/Abs.java

-    final Schema s = params.get(0);
-    if (!DecimalUtil.isDecimal(s)) {
+    final SqlType s = params.get(0);
+    if (!(s.baseType() == SqlBaseType.DECIMAL)) {


Suggested change

if (!(s.baseType() == SqlBaseType.DECIMAL)) {

if (s.baseType() != SqlBaseType.DECIMAL) {

vpapavas added 2 commits August 29, 2019 10:59

change public api of method schema provider to use SqlType instead of…

6eac04e

… Schema

Updated docs and tests

e173b0f

vpapavas requested review from JimGalasyn and a team as code owners August 29, 2019 21:11

vpapavas requested a review from agavra August 29, 2019 21:13

agavra approved these changes Aug 29, 2019

View reviewed changes

agavra requested a review from a team August 29, 2019 22:16

JimGalasyn approved these changes Aug 29, 2019

View reviewed changes

forgot test jar

4ccc75b

big-andy-coates approved these changes Sep 3, 2019

View reviewed changes

reverted pom changes, applied almog's comments

64b9847

vpapavas force-pushed the schema-provider branch from 32f4c8c to 64b9847 Compare September 3, 2019 17:11

applied Andy's comment

23ba289

vpapavas changed the title ~~improvement: Change the public API of schema provider method~~ improvement: change the public API of schema provider method Sep 4, 2019

vpapavas changed the title ~~improvement: change the public API of schema provider method~~ feat: change the public API of schema provider method Sep 4, 2019

vpapavas merged commit 1324285 into confluentinc:master Sep 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: change the public API of schema provider method #3287

feat: change the public API of schema provider method #3287

vpapavas commented Aug 29, 2019

ghost commented Aug 29, 2019

agavra left a comment

agavra Aug 29, 2019

big-andy-coates Sep 3, 2019

vpapavas Sep 3, 2019

agavra Aug 29, 2019

big-andy-coates Sep 3, 2019

agavra Aug 29, 2019

agavra Aug 29, 2019

big-andy-coates Sep 3, 2019

JimGalasyn left a comment

big-andy-coates left a comment

big-andy-coates Sep 3, 2019

big-andy-coates Sep 3, 2019

big-andy-coates Sep 3, 2019

big-andy-coates Sep 3, 2019

	if (!(s.baseType() == SqlBaseType.DECIMAL)) {
	if (s.baseType() != SqlBaseType.DECIMAL) {

feat: change the public API of schema provider method #3287

feat: change the public API of schema provider method #3287

Conversation

vpapavas commented Aug 29, 2019

Description

Testing done

Reviewer checklist

ghost commented Aug 29, 2019

agavra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimGalasyn left a comment

Choose a reason for hiding this comment

big-andy-coates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment