docs: klip-59: JSON functions #8550

Gerrrr · 2021-12-22T15:19:10Z

Description

KLIP for adding new JSON functions.

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

agavra

Nice! Thanks @Gerrrr

agavra · 2022-01-03T17:17:17Z

design-proposals/klip-59-json-functions.md

+Given a string, parses it as a JSON object and returns a ksqlDB array of two-element arrays
+representing the top-level keys and values. Returns `NULL` if the string can't be interpreted as a


json objects don't have to have the same value type, right? what would happen if we passed in {"a": 1, "b": "X"} (subtext: ksqlDB arrays are strongly typed)

Based on the example below, I think the result in ksql types would be: Array[Array['a', '1'], Array['b', '"X"']]. Note: double-quotes surrounding the value of b. Essentially each inner value (meaning the second item in the nested arrays) in the result is equivalent to calling to_json_string on the JSON value input.

I assume the relevant the UDF's return type is Array<Array<string>>.

Agree, the return type should be Array<Array<string>>.

We should update the KLIP to call out the exact type of the result array.

Btw: Why do we return an array of 2-element arrays? Would it be better to return an array of structs with two string field instead, ie, Array<Struct<json_key:String,json_value:String>>

This SO answer helped me see that things could be tricky: https://stackoverflow.com/a/53529204

I don't know what to recommend here. This is definitely a sharp edge.

What about duplicate keys?

Duplicat keys is either not an issue or a way bigger issue, depending on how we look at them. Jackson does not support duplicate keys, as ObjectNode maintains its fields in LinkedHashMap that does not allow duplicates (link) and it is unlikely that they will support them in the near future (FasterXML/jackson-databind#2809).

It seems like the options are:

Go with Array<Struct<String,String>> (since that's the existing code and it'd cover things if there was a desire to support the JSON spec to the letter).

Switch to Map<String, String> (since this is what the library supports).

Thoughts? At this point, I am unsure what to suggest.

Might be a product call, if we should follow the spec or the limitations of Jackson... \cc @colinhicks

We discussed this offline and agreed not to support duplicate keys for now and proceed with Map<String, String> return type here. I updated the doc in 85de9ab.

agavra · 2022-01-03T17:19:43Z

design-proposals/klip-59-json-functions.md

+* A JSON datatype
+* Binary JSON support


I don't immediately see any problems with omitting this from scope, but if we do ever introduce these would we expect the semantics of any of these to change at all (e.g. return a JSON type instead of a string or somethign else)?

I would rather introduce a separate set of functions or operators (see PostgreSQL syntax) for the JSON type instead of changing the return type of the existing functions. The reasons are:

We can provide functions for built-in JSON type with more elegant syntax AND avoid wrapping the output in strings.

Having a function that behaves differently given different argument types might confuse the users. Consider json_concat that kinda does it already. Its behavior description is three times longer than descriptions for any other proposed function, which is a good proxy for cognitive complexity.

If we would add a JSON type, how would we name functions that accept JSON compared to the function names that are proposed in this KLIP? Just curious.

Thinking about your question, I realized that function names we picked in this KLIP would actually be perfect for the JSON type. I propose to rename functions here in the following way:

is_json -> is_json_string. The former name would better correspond to a function that returns true if the argument is of JSON type.

json_array_length -> json_string_array_length

json_keys -> json_string_keys

json_records -> json_string_records

json_concat -> json_string_concat

Thoughts @mjsax @agavra @colinhicks @suhas-satish ?

What is the motivation for having different function names depending on the argument type? From an implementation perspective, if we were to implement a JSON type, wouldn't this just mean adding an overload that accepts an argument of that type?

If we have json_concat(string, string) -> string and json_concat(json, json) -> json it seems to be well defined?

I think that it might be confusing for users if a function returns a different type based on its arguments type.

Besides, this would be inconsistent with our current practice - e.g., we have ARRAY_UNION and MAP_UNION rather than a single function UNION that is overloaded for different containers.

I think that it might be confusing for users if a function returns a different type based on its arguments type.

Not really (at least not for me). And we do this already in ksqlDB. For example FLOOR() (https://github.com/confluentinc/ksql/blob/master/ksqldb-engine/src/main/java/io/confluent/ksql/function/udf/math/Floor.java) is defined for int,long,double,decimal and it's return type matches it's input type (many other function work similar).

I am not sure why we have ARRAY_UNION and MAP_UNION. It does not make too much sense to me to have both, instead of UNION() (even if UNION() is a little bit tricky as there is also the SQL UNION keyword -- even if we don't support UNION).

as the original author of MAP_UNION and ARRAY_UNION i will chime in to say that is exactly (one of) the reasons why: i.e. we don't want to introduce a function whose name is the same as a SQL keyword. Secondary reason is that it follows the naming convention established in other database-like systems with comparable functionality (Presto in this case, IIRC)

So are you saying you would prefer an overloaded json_concat or to add json_string_concat?

I agree that we should override json_concat for json and string types. Thank you for the discussion!

design-proposals/klip-59-json-functions.md

mjsax · 2022-01-06T23:33:03Z

design-proposals/klip-59-json-functions.md

+is_json("1") // returns true
+is_json("\"abc\"") // returns true
+is_json("null") // returns true
+is_json("") // returns false


For my own education: why does this return false?

An empty string does not contain valid JSON because it can't be decoded into any of the primitive or structured types JSON can represent.

So it's just not in the json spec? (I guess the spec could define what it mean if they wanted to, eg, "" == "null").

Yeah, it is not in the spec.

design-proposals/klip-59-json-functions.md

mjsax · 2022-01-06T23:46:36Z

design-proposals/klip-59-json-functions.md

+```
+json_concat("{\"a\": 1}", "{\"b\": 2}") // returns "{\"a\": 1, \"b\": 2}"
+json_concat("{\"a\": {\"3\": 4}}", "{\"a\": {\"5\": 6}}") // returns "{\"a\": {\"5\": 6}}"
+json_concat("{\"a\": {\"5\": 6}}", "{\"a\": {\"3\": 4}}") // returns "{\"a\": {\"3\": 4}}"


Example seems to be redundant to the previous one in L165?

This was non-intuitive to me. I would have expected union of json values as the result. ie, "{"a": [{"3": 4}, {"5": 6}] }". Is this too difficult and error-prone to implement and not how its done in postgres?

@mjsax removed in 41e8e75.

@suhas-satish In my experience, this behavior for map union is widespread. Some examples are:

Python union operator

Java Map's putAll

Javascript's union

ksqlDB's MAP_UNION

@suhas-satish I agree that it not totally intuitive, but we don't need to have two example for the same case. L165 and L166 are just the same.

mjsax · 2022-01-06T23:49:56Z

design-proposals/klip-59-json-functions.md

+json_concat("{\"a\": {\"3\": 4}}", "{\"a\": {\"5\": 6}}") // returns "{\"a\": {\"5\": 6}}"
+json_concat("{\"a\": {\"5\": 6}}", "{\"a\": {\"3\": 4}}") // returns "{\"a\": {\"3\": 4}}"
+json_concat("{}", "{}") // returns "{}"
+json_concat("[1, 2]", "[3, 4]") // returns "[1,2,3,4]"


Seems this case is described via second bullet-point above -- but it's not really explicit. Should we update the bullet points above to be easier to follow?

Added a separate bullet point for the case when both strings deserialize into lists in f519ea9.

design-proposals/klip-59-json-functions.md

mjsax · 2022-01-06T23:58:12Z

design-proposals/klip-59-json-functions.md

+
+Given any ksqlDB type returns the equivalent JSON string.
+
+#### Examples


Can we add examples for all types? Missing: boolean, time, date, timestamp.

What about custom types? (Guess it's straightforward, but maybe worth to mention).

Might also be good to have a few example with nested complex types?

Added in 4cd7926.

design-proposals/klip-59-json-functions.md

Gerrrr · 2022-01-18T14:07:23Z

While implementing the first functions from this KLIP, I noticed that existing functions throw on deserialization errors. I updated the KLIP so that new functions are consistent with the existing ones - 515f8f5.

mjsax · 2022-01-19T23:10:01Z

I guess from a user point of view, it does not really make a difference if a function returns null or throws an exception? SQL does not support exceptions, and errors (like exceptions) result in NULL for the user? -- For the purpose of a KLIP, it seems that throwing an exception is an implementation detail?

Gerrrr · 2022-01-20T09:11:58Z

Good point @mjsax! I will revert the change.

jnh5y · 2022-01-24T18:55:52Z

design-proposals/klip-59-json-functions.md

+### json_concat
+
+```
+json_concat(json_string, json_string) -> String


(Aleks suggested I comment here.). Should/could this method take varargs?

As one thing to think about, should we have a UDAF to concat multiple entries? (Which would/should likely be a separate function.)

I think this is great idea. ksqlDB's UDFs support varargs, so we can just change this method to json_concat(json_strings...) without any downsides.

If the UDF frameworks supports it, sure!

Gerrrr · 2022-01-27T09:33:27Z

I am going to merge the PR; happy to continue the discussion here and create follow-up PRs if necessary.

design-proposals/klip-59-json-functions.md

Gerrrr force-pushed the docs-json-functions-klip branch from 7aa72b4 to bca057e Compare December 22, 2021 15:30

Gerrrr marked this pull request as ready for review December 22, 2021 15:30

Gerrrr requested a review from a team as a code owner December 22, 2021 15:30

Gerrrr requested review from colinhicks and mjsax December 22, 2021 15:31

agavra approved these changes Jan 3, 2022

View reviewed changes

colinhicks approved these changes Jan 5, 2022

View reviewed changes

mjsax reviewed Jan 6, 2022

View reviewed changes

design-proposals/klip-59-json-functions.md Outdated Show resolved Hide resolved

mjsax reviewed Jan 6, 2022

View reviewed changes

design-proposals/klip-59-json-functions.md Outdated Show resolved Hide resolved

mjsax reviewed Jan 6, 2022

View reviewed changes

design-proposals/klip-59-json-functions.md Show resolved Hide resolved

mjsax reviewed Jan 6, 2022

View reviewed changes

design-proposals/klip-59-json-functions.md Outdated Show resolved Hide resolved

mjsax reviewed Jan 6, 2022

View reviewed changes

design-proposals/klip-59-json-functions.md Outdated Show resolved Hide resolved

mjsax reviewed Jan 6, 2022

View reviewed changes

mjsax reviewed Jan 7, 2022

View reviewed changes

design-proposals/klip-59-json-functions.md Outdated Show resolved Hide resolved

mjsax reviewed Jan 7, 2022

View reviewed changes

design-proposals/klip-59-json-functions.md Outdated Show resolved Hide resolved

mjsax approved these changes Jan 20, 2022

View reviewed changes

Gerrrr mentioned this pull request Jan 21, 2022

feat: Add json_array_length UDF #8602

Merged

2 tasks

Gerrrr force-pushed the docs-json-functions-klip branch from 5538b98 to a3d1430 Compare January 21, 2022 13:41

Gerrrr mentioned this pull request Jan 21, 2022

feat: Add json_records, to_json_string, and json_concat UDFs #8632

Merged

2 tasks

docs: klip-59: JSON functions

50efc86

Gerrrr force-pushed the docs-json-functions-klip branch from a3d1430 to 50efc86 Compare January 21, 2022 16:00

jnh5y reviewed Jan 24, 2022

View reviewed changes

Make json_records return map<string,string>

85de9ab

Gerrrr merged commit 76f02b7 into master Jan 27, 2022

IvanVas reviewed Feb 8, 2022

View reviewed changes

design-proposals/klip-59-json-functions.md Show resolved Hide resolved

jnh5y deleted the docs-json-functions-klip branch February 10, 2022 20:07

		Given a string, parses it as a JSON object and returns a ksqlDB array of two-element arrays
		representing the top-level keys and values. Returns `NULL` if the string can't be interpreted as a


		Given any ksqlDB type returns the equivalent JSON string.

		#### Examples

docs: klip-59: JSON functions #8550

docs: klip-59: JSON functions #8550

Conversation

Gerrrr commented Dec 22, 2021

Description

Reviewer checklist

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

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

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

Gerrrr Jan 7, 2022 • edited Loading

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

Gerrrr commented Jan 18, 2022

mjsax commented Jan 19, 2022

Gerrrr commented Jan 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gerrrr commented Jan 27, 2022

Gerrrr Jan 7, 2022 •

edited

Loading