Add support for `json_object` function #664

jorgehermo9 · 2025-01-13T00:12:30Z

Relates to #127. This PR is still in draft and I have a few left things to do (tests, improve implementation), just opening it so anyone can track this work meanwhile.

jorgehermo9 · 2025-01-15T21:19:59Z

core/json/mod.rs

@@ -85,6 +85,8 @@ pub fn json_array(values: &[OwnedValue]) -> crate::Result<OwnedValue> {
    let mut s = String::new();
    s.push('[');

+    // TODO: use `convert_db_type_to_json` and map each value with that function,


This function seems to do the same as convert_db_type_to_json and therefore it can be simplified a lot. I can deal with this refactor in a follow-up PR. I think it would lead to more simple array construction than pushing strings.

jorgehermo9 · 2025-01-15T21:31:49Z

core/json/mod.rs

+                // it was obtained from another JSON function.
+                // TODO: the `json_array` function should use something like this.
+                // That implementation could be a lot simpler with this function
+                let json_val = convert_db_type_to_json(value)?;


with convert_db_type_to_json we convert the treat the values as a JSON object only if
it was obtained from another JSON function.

This matches sqlite's behaviour: https://www.sqlite.org/json1.html#jobj

An argument with SQL type TEXT it is normally converted into a quoted JSON string even if the input text is well-formed JSON. However, if the argument is the direct result from another JSON function or the -> operator (but not the ->> operator), then it is treated as JSON and all of its JSON type information and substructure is preserved. This allows calls to json_object() and json_array() to be nested. The json() function can also be used to force strings to be recognized as JSON.

the json_array function should use something like this. That implementation could be a lot simpler with this function https://github.com/tursodatabase/limbo/pull/664/files#r1917348517

jorgehermo9 · 2025-01-15T21:33:00Z

core/json/mod.rs

+                    // serde's `rc` feature so we can serialize Rc<String>
+                    OwnedValue::Text(t) => t.value.to_string(),
+                    // TODO: I matched sqlite message error here. Is this ok?
+                    _ => crate::bail_constraint_error!("labels must be TEXT"),


I matched sqlite's message error in this case. Is this ok?

jorgehermo9 · 2025-01-15T21:34:13Z

core/json/mod.rs

+                let key = match key {
+                    // TODO: We can construct the IndexMap from Rc<String>, but we must enable the
+                    // serde's `rc` feature so we can serialize Rc<String>
+                    OwnedValue::Text(t) => t.value.to_string(),


had to use String instead of Rc<String> because of the IndexMap<String,Val> serialization. We can't serialize an IndexMap<Rc<String>,Val> if we don't have the rc feature of serde enabled.
Should be better to add that feature to avoid cloning the string here?
https://serde.rs/feature-flags.html#rc

jorgehermo9 · 2025-01-15T21:35:45Z

core/json/mod.rs

+/// the returned `Val` will be an object. If the internal text value is a regular text,
+/// then a string will be returned. This is useful to track if the value came from a json
+/// function and therefore we must interpret it as json instead of raw text when working with it.
+fn convert_db_type_to_json(value: &OwnedValue) -> crate::Result<Val> {


Would it be better to use a impl TryFrom<&OwnedValue> for Val? Seems more idiomatic

(the same goes to convert_json_to_db_type function a few lines above)

I can address this refactor in another PR if it makes sense

I think it's a good idea!

perfect! will do it in a follow-up PR

jorgehermo9 · 2025-01-15T21:38:30Z

core/json/mod.rs

+            _ => crate::bail_constraint_error!("json_object requires an even number of values"),
+        })
+        // TODO: collecting into a IndexMap does not allow for repeated keys
+        .collect::<Result<IndexMap<String, Val>, _>>()?;


Collecting into an IndexMap does not allow to have repeated keys. Sqlite documentation about this is that:

The json_object() function currently allows duplicate labels without complaint, though this might change in a future enhancement.

What is the behaviour we should have here? I think it does not make sense to have repeated keys in JSON.

jorgehermo9 · 2025-01-15T21:39:30Z

core/json/mod.rs

+    }
+
+    #[test]
+    fn test_json_object_duplicated_keys() {


This checks the behaviour from https://github.com/tursodatabase/limbo/pull/664/files#r1917368455

jorgehermo9 · 2025-01-15T21:39:45Z

testing/json.test

+    SELECT json(json_object('key','value'));
+} {{{"key":"value"}}}
+
+# FIXME: this behaviour differs from sqlite. Although, sqlite docs states


As commented in https://github.com/tursodatabase/limbo/pull/664/files#r1917368455. How should we proceed here?

I'd actually expect this test to pass as-is, ie it should return {"key":"value2"}. Any application will eventually convert this object to a valid JSON and duplicate the keys anyway.

yep, I agree.

The problem is, sqlite outputs {"key:"value","key":"value2"} and this test won't pass in CI

jorgehermo9 · 2025-01-15T21:41:19Z

testing/json.test

@@ -506,3 +506,41 @@ do_execsql_test json_error_position_null {
 do_execsql_test json_error_position_complex {
  SELECT json_error_position('{a:null,{"h":[1,[1,2,3]],"j":"abc"}:true}');
 } {{9}}
+
+do_execsql_test json_object_simple {


Inspired from sqlite's tests about this function https://github.com/sqlite/sqlite/blob/f1747f93e0f8df7984b595b91649c7789217fe59/test/json102.test#L21

But did not copy them entirely. Are those enough test cases or should I include more from sqlite's test suite?

jorgehermo9 · 2025-01-15T21:42:54Z

core/translate/expr.rs

+                $func.to_string()
+            );
+        };
+        // The only function right now that requires an even number is `json_object` and it allows


I can remove those comments if required but seemed a bit confusing that this behaviour differs from expect_arguments_max and expected_arguments_min.

Maybe writing a macro for this single case is a bit overengineered, was thinking about futures uses of this. I can remove this macro and extend it in L846 if required

jorgehermo9 · 2025-01-15T21:44:01Z

I'm ready for a review. Left a feed comments about some doubts I had, as I'm new in the project and I don't know much about the codebase & how you work.

Thank you very much!!

jorgehermo9 · 2025-01-15T23:08:52Z

COMPAT.md

@@ -270,7 +270,7 @@ Feature support of [sqlite expr syntax](https://www.sqlite.org/lang_expr.html).
 | json ->> path                      | Yes     |                                                                                                                                              |
 | json_insert(json,path,value,...)   |         |                                                                                                                                              |
 | jsonb_insert(json,path,value,...)  |         |                                                                                                                                              |
-| json_object(label1,value1,...)     |         |                                                                                                                                              |
+| json_object(label1,value1,...)     | Yes     | When keys are duplicated, only the last one processed is returned. This differs from sqlite, where the keys in the output can be duplicated  |


Is this note ok? Or should I work to fix that?

https://github.com/tursodatabase/limbo/pull/664/files#r1917368455

core/json/mod.rs

jorgehermo9 added 11 commits January 12, 2025 22:29

feat: initial json_object implementation

c7b8315

feat: initial json_object implementation

3785e7c

feat: initial json_object implementation

e01b8f8

chore: add todo

06208af

Merge with main

c1702b3

fix: compilation in CI

2056691

test: add tests for json_object

a4cab8e

test: add TCL tests for json_object

fea8d19

docs: update COMPAT.md

b3d3c8c

Merge with main

fa8eb95

Merge with main

cd16a19