Json extract #524

madejejej · 2024-12-20T13:52:39Z

This is currently a WIP and rebased on top of #504

madejejej · 2024-12-20T13:54:49Z

I hoped this feature wouldn't drop me into a rabbit hole 😅 Unfortunately, there are some quirks in the JSON syntax, ex:

-- "\x61" is an escaped ASCII character for 'a', but this doesn't match in neither sqlite nor postgres:
SELECT json_extract('{"\x61": 1}', 'a');

-- you need to use the exact sequence of characters:
SELECT json_extract('{"\x61": 1}', '\x61')
1

-- the other way around also wouldn't work:
SELECT json_extract('{"a": 1}', '\x61')

madejejej · 2024-12-20T13:58:10Z

core/json/json_path.rs

+
+#[derive(Parser)]
+#[grammar_inline = r#"
+array_locator = @{ "[" ~ array_index ~ "]" }


I had to copy a large part of grammar from core/json/de.rs. Perhaps I could extract the grammar from there into a constant and add additional rules for parsing the path? However, this would leave us with a few unused rules.

The other way to DRY it out would be to extract only the common rules, but I feel like it's brittle and very unhandy if we ever have to patch the JSON grammar.

jussisaurio · 2024-12-20T14:38:10Z

i merged #504 so base is now main

madejejej · 2024-12-31T08:41:50Z

core/json/de.rs

@@ -9,137 +9,7 @@ use std::f64;
 use crate::json::error::{self, Error, Result};

 #[derive(Parser)]
-#[grammar_inline = r#"


moved to a separate file

madejejej · 2024-12-31T08:48:38Z

@petersooley I saw your PR with json_array_length got merged while I was working on json_extract.

I like the idea of a smaller, hand-rolled parser but I also think the JSON grammar is pretty complicated, even in terms of what you can do with the JSON path in SQLite (see some tests attached). Let me know your thoughts.

madejejej · 2024-12-31T08:54:59Z

Test FAILED: 'SELECT json_extract(1, null, null, null)'
returned ''
expected '[null,null,null]'

SQLite seems to be changing some quirky behavior from version to version:

sqlite> .version
SQLite 3.43.2 2023-10-10 13:08:14 1b37c146ee9ebb7acd0160c0ab1fd11017a419fa8a3187386ed8cb32b709aapl
zlib version 1.2.12
clang-15.0.0 (64-bit)
sqlite> SELECT json_extract(1, null, null, null);
[null,null,null]

jussisaurio · 2024-12-31T12:27:15Z

core/json/json.pest

+// see https://spec.json5.org/#syntactic-grammar and
+// https://spec.json5.org/#lexical-grammar
+
+COMMENT = _{ "/*" ~ (!"*/" ~ ANY)* ~ "*/" | "//" ~ (!line_terminator ~ ANY)* }


Was this grammar hand-written based on the above spec or sourced from somewhere else?

nvm, this file was moved instead of created here, so you might not know

jussisaurio · 2024-12-31T12:29:08Z

core/json/json_path.rs

+struct Parser;
+
+#[derive(Clone, Debug, PartialEq)]
+pub enum PathElement {


Could you also comment these using triple-slash comments? Each enum variant / struct field can also have its own triple-slash comment. IDEs pick those up which is helpful

jussisaurio · 2024-12-31T12:29:16Z

core/json/json_path.rs

+    pub elements: Vec<PathElement>,
+}
+
+// Parses path into a Vec of Strings, where each string is a key or an array locator.


Triple slash, please

jussisaurio · 2024-12-31T12:31:28Z

core/json/json_path.rs

+                        ),
+                    }
+                }
+                _ => {


can this branch be hit? doesn't the parser produce discreet unions where we can handle every possible union variant?

jussisaurio · 2024-12-31T12:31:58Z

core/json/json_path.rs

+            match pair.as_rule() {
+                Rule::EOI => (),
+                Rule::root => result.push(PathElement::Root()),
+                Rule::json_path_key => result.push(PathElement::Key(pair.as_str().to_string())),


Can these be made into pascal case? Rule::JsonPathKey

jussisaurio · 2024-12-31T12:33:09Z

core/json/json_path.rs

+                        Rule::negative_index_indicator => {
+                            let negative_offset = array_locator.next().unwrap();
+                            // TODO: sqlite is able to parse arbitrarily big numbers, but they
+                            //  always get overflown and cast to i32. Handle this.


any reason not to handle it in this PR?

jussisaurio · 2024-12-31T12:40:49Z

testing/json.test

+    SELECT json_extract(1, '$')
+} {{1}}
+
+# TODO: fix me


If we plan to keep these commented out when merging this, please make the comments descriptive enough that anyone can pick them up and work on them

jussisaurio · 2024-12-31T12:45:12Z

core/vdbe/mod.rs

+                                _ => {
+                                    let val = &state.registers[*start_reg];
+                                    let reg_values =
+                                        &state.registers[*start_reg + 1..*start_reg + arg_count];


i think this range should be ..= instead of ..

consider:

// start_reg = 0 // arg_count = 2 let reg_values = &state.registers[1..2]; // <-- only picks one arg since range end is not inclusive

In fact there seem to be no tests for json_extract() with multiple valid paths - should probably add some?

jussisaurio · 2024-12-31T12:56:48Z

core/json/mod.rs

+
+    for path in paths {
+        match path {
+            OwnedValue::Text(p) => {


I feel like this would be simpler if json_extract_single took an &OwnedValue instead of str so we wouldn't have to pattern match here at all.

let mut result = "".to_string(); if paths.len() > 1 { result.push('['); } for path in paths.iter() { let extracted = json_extract_single(&json, path); if paths.len() == 1 && extracted == Val::Null { return OwnedValue::Null; } result.push_str(&crate::json::to_string(&extracted).unwrap()); }

etc

jussisaurio · 2024-12-31T12:57:13Z

core/json/mod.rs

+    }
+
+    if paths.len() > 1 {
+        result.pop();


can you add a comment saying that this removes the final comma

petersooley · 2024-12-31T18:03:37Z

@petersooley I saw your PR with json_array_length got merged while I was working on json_extract.

I like the idea of a smaller, hand-rolled parser but I also think the JSON grammar is pretty complicated, even in terms of what you can do with the JSON path in SQLite (see some tests attached). Let me know your thoughts.

@madejejej The path parsing is much simpler than the JSON parsing, for sure. It's also very limited in sqlite (i.e. no glob/wildcard patterns). It's mostly array indexes and object property paths with a few extra cases.

What I like about the hand-rolled solution is that it doesn't separate out the path parsing from accessing the value in the JSON. That allows returning early as soon as the JSON value has no match for the path. No matter which way we go, there's always a loop required to drill into the JSON and extract a value at the end of the given path. Both solutions are doing that loop anyway, it's just that the hand-rolled solution is doing it during path parsing.

jussisaurio · 2024-12-31T20:32:12Z

@petersooley I saw your PR with json_array_length got merged while I was working on json_extract.
I like the idea of a smaller, hand-rolled parser but I also think the JSON grammar is pretty complicated, even in terms of what you can do with the JSON path in SQLite (see some tests attached). Let me know your thoughts.

@madejejej The path parsing is much simpler than the JSON parsing, for sure. It's also very limited in sqlite (i.e. no glob/wildcard patterns). It's mostly array indexes and object property paths with a few extra cases.

What I like about the hand-rolled solution is that it doesn't separate out the path parsing from accessing the value in the JSON. That allows returning early as soon as the JSON value has no match for the path. No matter which way we go, there's always a loop required to drill into the JSON and extract a value at the end of the given path. Both solutions are doing that loop anyway, it's just that the hand-rolled solution is doing it during path parsing.

Yeah this is a decent point, we'd save some deserialization overhead by traversing the JSON object on demand while parsing the path. Maybe not the most important thing in the world and can be optimized later, but it's also nice to implement things right the first time around. I wouldn't block this PR from going forward with the eagerly-parsed version so I'll let @madejejej decide

madejejej commented Dec 20, 2024

View reviewed changes

madejejej force-pushed the json-extract branch from 84f74cc to b401178 Compare December 20, 2024 16:28

Add support for json_extract

ad9acf7

madejejej force-pushed the json-extract branch 2 times, most recently from cc089af to 332b831 Compare December 31, 2024 08:40

Merge branch 'main' into json-extract

2e730ea

madejejej force-pushed the json-extract branch from 332b831 to 2e730ea Compare December 31, 2024 08:41

madejejej commented Dec 31, 2024

View reviewed changes

madejejej force-pushed the json-extract branch from 6a5d800 to eb5bbe6 Compare December 31, 2024 08:44

madejejej force-pushed the json-extract branch 3 times, most recently from 0b639f4 to d970d00 Compare December 31, 2024 08:52

Merge branch 'main' into json-extract

692301e

madejejej force-pushed the json-extract branch from d970d00 to 692301e Compare December 31, 2024 08:53

Mark tests to fix for newer SQLite version

719eda7

madejejej marked this pull request as ready for review December 31, 2024 09:00

jussisaurio reviewed Dec 31, 2024

View reviewed changes

core/json/mod.rs

}

if paths.len() > 1 {

result.pop();

Copy link

Collaborator

jussisaurio Dec 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment saying that this removes the final comma

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Json extract #524

Json extract #524

madejejej commented Dec 20, 2024

madejejej commented Dec 20, 2024

madejejej Dec 20, 2024

jussisaurio commented Dec 20, 2024

madejejej Dec 31, 2024

madejejej commented Dec 31, 2024

madejejej commented Dec 31, 2024

jussisaurio Dec 31, 2024

jussisaurio Dec 31, 2024

jussisaurio Dec 31, 2024

jussisaurio Dec 31, 2024

jussisaurio Dec 31, 2024

jussisaurio Dec 31, 2024

jussisaurio Dec 31, 2024

jussisaurio Dec 31, 2024

jussisaurio Dec 31, 2024 •

edited

Loading

jussisaurio Dec 31, 2024

jussisaurio Dec 31, 2024

petersooley commented Dec 31, 2024

jussisaurio commented Dec 31, 2024

Json extract #524

Are you sure you want to change the base?

Json extract #524

Conversation

madejejej commented Dec 20, 2024

madejejej commented Dec 20, 2024

Choose a reason for hiding this comment

jussisaurio commented Dec 20, 2024

Choose a reason for hiding this comment

madejejej commented Dec 31, 2024

madejejej commented Dec 31, 2024

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

jussisaurio Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petersooley commented Dec 31, 2024

jussisaurio commented Dec 31, 2024

jussisaurio Dec 31, 2024 •

edited

Loading