json unfolding: parse incrementally instead of using cheshire #42638

piranha · 2024-05-14T12:38:06Z

This lowers memory and time usage 100x for 50kb strings (our previous limit). For example, those are results for parsing 42kb JSON string (already in memory):

Before: Time per call: 6.32 ms    Alloc per call: 8,732,850b
After:  Time per call: 55.16 us   Alloc per call: 83,254b

Nuances:

I've dropped 50k limit, but not 100 keys limit; so in theory some weird really big JSON with low amount of keys could take a bit of time; in practice, it takes about 455ms and 147mb of allocations to go through 13Mb JSON file. Seems acceptable.
We could descend into (top-level only?) arrays, but I have no understanding on how to represent them in a resulting sequence; any pointers?
It seems that outside of enabling this option specifically, there is no way for Jackson to read float from JSON as BigDecimal; OTOH I don't see cheshire reading them as such, so I'm not too sure which way to go.

Resolves #34798

this lowers memory usage 100x for 50kb strings (our previous limit) and speeds up parsing by an order of magnitude

replay-io · 2024-05-14T12:50:27Z

Status	Complete ↗︎
Commit	`fcf7a89`
Results	⚠️ 2 Flaky ✅ 2506 Passed

crisptrutski · 2024-05-14T13:04:32Z

We could descend into (top-level only?) arrays, but I have no understanding on how to represent them in a resulting sequence; any pointers?

If you're not worried about representing empty arrays, then you can just represent them implicitly by having index integers in the paths of their values.

crisptrutski

It would be good to add some guard rails to the case macro, but don't want to hold this up.

crisptrutski · 2024-05-14T13:07:31Z

src/metabase/driver/sql_jdbc/sync/describe_table.clj

+    JsonParser$NumberType/INT Integer
+    JsonParser$NumberType/LONG Long
+    JsonParser$NumberType/FLOAT Float
+    JsonParser$NumberType/DOUBLE Double
+    JsonParser$NumberType/BIG_DECIMAL BigDecimal
+    JsonParser$NumberType/BIG_INTEGER clojure.lang.BigInt))


Why do we care about exposing Integer and Float? We could be opinionated like Clojure and just promote them.

Hmmm, maybe; I've tried to repeat whatever behavior we had in place and was just eager to finally get PR up. 😁 I'll look into making this simpler.

crisptrutski · 2024-05-14T13:11:10Z

src/metabase/util.cljc

+       ~@(concat
+          (mapcat (fn [[test result]]
+                    #_ {:clj-kondo/ignore [:discouraged-var]}
+                    [(eval (enum-ordinal test)) result])


Damn this is a pity. Wonder whether we can't use reflection on the class instead?

We should probably also ensure that all the dispatch enums and the input enum have the same type.

Probably jumping the shark a bit, but it would be nice if we could effectively generate a Java switch statement - especially cool if we enforce exhaustiveness too.

Oh yeah generating switch would be cool, but absolutely no idea how to get there. As for dropping eval - if you have any ideas, I'm all ears. :)

added some checks/tests

test/metabase/driver/sql_jdbc/sync/describe_table_test.clj

src/metabase/driver/sql_jdbc/sync/describe_table.clj

crisptrutski · 2024-05-14T20:29:46Z

src/metabase/driver/sql_jdbc/sync/describe_table.clj

+          ;; we could be more precise here and issue warning about nested fields (the one in `describe-json-fields`),
+          ;; but this limit could be hit by multiple json fields rather than only by this one. So for the sake of
+          ;; issuing only a single warning in logs we'll spill over limit by a single entry (instead of doing `<=`).


This is a some subtle stuff - I had to read it twice to get the gist and, this is a bit embarrassing, I still don't see how we can hit it multiple times. Don't we stop reading tokens and then pop off the entire loop's tail call stack?

I think it's worth explicitly saying that you're leaving the item to trigger the warning later, and that the last¹ item will be lopped off again then too.

It's a bit hacky, but I think you could avoid this trick by only logging if res is still a transient, although that means reflection. Alternatively you could use another loop parameter as a latch.

Footnotes

Well, a random item will be removed. A pity that we forget the order by then. ↩

I still don't see how we can hit it multiple times

If there are a few fields with JSON, even if you got them under the limit - they could be over 100 keys in total. So not in this function directly, I'll probably add some more words here.

Ah gotcha. I hadn't looked much at the surrounding code.

crisptrutski · 2024-05-14T20:32:03Z

test/metabase/driver/sql_jdbc/sync/describe_table_test.clj

+    (let [data (into {} (for [i (range (* 2 @#'sql-jdbc.describe-table/max-nested-field-columns))]
+                          [(str "key" i) i]))]
+      ;; +2 to limit since we go 1 over the limit, see comment in `json->types`
+      (is (> (+ 2 @#'sql-jdbc.describe-table/max-nested-field-columns)


Why not test exactly what size it is? I think it's fine to update the test if we fix the "1 over" thing later.

crisptrutski · 2024-05-14T20:33:12Z

test/metabase/util_test.cljc

+       (is (not (case Month/MAY
+                  Month/APRIL false
+                  Month/MAY   true
+                  false))))


nit: I'd prefer this test with distinct values for all three branches so it's clearer.

Suggested change

(is (not (case Month/MAY

Month/APRIL false

Month/MAY true

false))))

(is (= 3 (case Month/MAY

Month/APRIL 1

Month/MAY 2

3))))

crisptrutski

Nice improvements, double ticked

crisptrutski · 2024-05-14T20:36:38Z

src/metabase/util.cljc

+     "Like `case`, but explicitly dispatch on Java enum ordinals."
+     [value & clauses]
+     #_ {:clj-kondo/ignore [:discouraged-var]}
+     (let [types (map (comp type eval first) (partition 2 clauses))]


Isn't there another way to look up the type from a symbol? 😢

I found this 🤢

(let [sym 'JsonToken/VALUE_STRING] (clojure.lang.Reflector/invokeStaticMethod (resolve (symbol (namespace sym))) "valueOf" (to-array [(name sym)])))

Still better than eval though, I think.

(defn sym->enum [s] 🤮)

as they say, be wrong on the internet and learn 😁

crisptrutski · 2024-05-14T20:39:35Z

test/metabase/util_test.cljc

+             Month/MAY   true
+             false)))
+     (testing "checks for type"
+       (is (thrown? Exception #"`case-enum` only works.*"


It's great that we check this, but it still leaves the slightly more likely bug which is having a mismatch just in runtime value we take, e.g.

(u/case-enum DayOfWeek/SUNDAY Month/JANUARY 1)

We can't protect against this without reflection or a more advanced technique, so I think this is fine, but perhaps worth a test documenting the sad as well.

crisptrutski · 2024-05-14T21:02:30Z

src/metabase/util.cljc

+              ~@(concat
+                 (mapcat (fn [[test result]]
+                           #_ {:clj-kondo/ignore [:discouraged-var]}
+                           [(eval (enum-ordinal test)) result])


So I think this can then be

[(.ordinal (sym->enum test)) result])

PS: do you know how to make inline code fragments use Clojure syntax highlighting?

crisptrutski · 2024-05-15T09:20:26Z

src/metabase/util.cljc

+
+       Passing the same enum type as the ones you're checking in is on you, this is not checked."
+       [value & clauses]
+       (let [types (map (comp type sym->enum first) (partition 2 clauses))]


nit: You could calculate the type a bit more directly with just (comp resolve symbol namespace first)

github-actions · 2024-05-15T10:08:44Z

@piranha Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

this lowers memory usage 100x for 50kb strings (our limit before this change) and speeds up parsing by an order of magnitude. For example, results for parsing 42kb string (already in memory): Before: Time per call: 6.32 ms Alloc per call: 8,732,850b After: Time per call: 55.16 us Alloc per call: 83,254b

#42682) this lowers memory usage 100x for 50kb strings (our limit before this change) and speeds up parsing by an order of magnitude. For example, results for parsing 42kb string (already in memory): Before: Time per call: 6.32 ms Alloc per call: 8,732,850b After: Time per call: 55.16 us Alloc per call: 83,254b

json unfolding: parse incrementally instead of using cheshire

2579e4f

this lowers memory usage 100x for 50kb strings (our previous limit) and speeds up parsing by an order of magnitude

piranha added the backport Automatically create PR on current release branch on merge label May 14, 2024

piranha requested a review from a team May 14, 2024 12:38

piranha self-assigned this May 14, 2024

piranha requested a review from camsaul as a code owner May 14, 2024 12:38

metabase-bot bot added the .Team/BackendComponents also known as BEC label May 14, 2024

crisptrutski approved these changes May 14, 2024

View reviewed changes

piranha added 2 commits May 14, 2024 17:45

write tests for case-enum

bdeebe4

test limit of keys

32faa8c

crisptrutski reviewed May 14, 2024

View reviewed changes

crisptrutski approved these changes May 14, 2024

View reviewed changes

crisptrutski reviewed May 14, 2024

View reviewed changes

piranha added 2 commits May 15, 2024 11:15

Chris found a good way to eschew eval

1a522bc

simpler type hierarchy

fcf7a89

crisptrutski reviewed May 15, 2024

View reviewed changes

piranha merged commit 5cfc079 into master May 15, 2024
109 checks passed

piranha deleted the json-unfolding branch May 15, 2024 10:08

metabase-bot bot mentioned this pull request May 15, 2024

🤖 backported "json unfolding: parse incrementally instead of using cheshire" #42682

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

json unfolding: parse incrementally instead of using cheshire #42638

json unfolding: parse incrementally instead of using cheshire #42638

piranha commented May 14, 2024

replay-io bot commented May 14, 2024 •

edited

crisptrutski commented May 14, 2024

crisptrutski left a comment

crisptrutski May 14, 2024

piranha May 14, 2024

crisptrutski May 14, 2024

crisptrutski May 14, 2024

piranha May 14, 2024

piranha May 14, 2024

crisptrutski May 14, 2024

piranha May 15, 2024

crisptrutski May 15, 2024

crisptrutski May 14, 2024

crisptrutski May 14, 2024

crisptrutski left a comment

crisptrutski May 14, 2024

crisptrutski May 14, 2024 •

edited

crisptrutski May 14, 2024 •

edited

piranha May 15, 2024

crisptrutski May 14, 2024

crisptrutski May 14, 2024

crisptrutski May 15, 2024

github-actions bot commented May 15, 2024

json unfolding: parse incrementally instead of using cheshire #42638

json unfolding: parse incrementally instead of using cheshire #42638

Conversation

piranha commented May 14, 2024

replay-io bot commented May 14, 2024 • edited

crisptrutski commented May 14, 2024

crisptrutski 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

Footnotes

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

crisptrutski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisptrutski May 14, 2024 • edited

Choose a reason for hiding this comment

crisptrutski May 14, 2024 • edited

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

github-actions bot commented May 15, 2024

replay-io bot commented May 14, 2024 •

edited

crisptrutski May 14, 2024 •

edited

crisptrutski May 14, 2024 •

edited