-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(hogvm): json parsing #22705
feat(hogvm): json parsing #22705
Conversation
Size Change: +198 B (+0.02%) Total Size: 1.06 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR description will be referenced in university lectures one day ;)
}, | ||
jsonStringify: (args) => { | ||
// Recursively convert maps to objects | ||
function convert(x: any): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intrigued to know the perf of this. At some point we probably do want some sort of tests measuring that kind of thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might have some performance drawbacks, and I hope we get everything else right and can look at benchmarking these things.
Some thoughts regarding performance for now:
- We might rewrite everything in a compiled language anyway, so a boost here might be 🤷
new Map([[key1, value1], [key2, value2]])
and vice versa might be a lot faster?- Making our own JSONify function (a loop that concatenates strings) may be even faster?
We can also switch HogVM to use objects natively instead of maps. I went with objects initially, but swapped to maps because 1) simple add/remove operations are 10x faster on maps vs objects, 2) objects in JS only support string keys, maps support anything as keys... just like python's dicts. So moving from objects to maps internally made the entire thing faster and more compatible with the Python version. However yes, I didn't think of a potential performance drawback with json.stringify. Let's see how far we can get with this? 🤷
Problem
Websites like JSON, so we should support it in Hog.
Changes
Adds two functions:
jsonParse(string)
andjsonStringify(any, indent=0)
that do just that.Naming things
I'm very open to good name suggestions. I took inspiration from JavaScript due to its familiarity.
parse
is a commonly used term, butstringify
is JS-only. We're definitely not going for Python'sjson.💩s
approach.toJSON
andfromJSON
are confusing because you never know what is from and what is to. I thoughttoString
could also stand in forstringifyJSON
, but while it would work with objects, you don't expect extra quotes wen you call it ontoString('a normal string')
.Here are what other languages do:
JavaScript
JSON.stringify(object)
JSON.parse(jsonString)
Python
json.dumps(object)
json.loads(jsonString)
Java
new Gson().toJson(object)
(using Gson library)new Gson().fromJson(jsonString, Class<T>)
(using Gson library)C#
JsonConvert.SerializeObject(object)
(using Newtonsoft.Json)JsonConvert.DeserializeObject<T>(jsonString)
(using Newtonsoft.Json)Ruby
object.to_json
JSON.parse(jsonString)
PHP
json_encode(object)
json_decode(jsonString, true)
Go
json.Marshal(object)
json.Unmarshal(jsonString, &object)
Swift
try JSONEncoder().encode(object)
try JSONDecoder().decode(Type.self, from: jsonString)
Kotlin
Gson().toJson(object)
(using Gson library)Gson().fromJson(jsonString, Type::class.java)
(using Gson library)Rust
serde_json::to_string(&object).unwrap()
serde_json::from_str(jsonString).unwrap()
so... dunno.
jsonParse(string)
andjsonStringify(object, indent=0)
<-- we have this nowparseJSON(string)
andstringifyJSON(object, indent=0)
<-- alternative, but writing capital JSON every time already became annoying, plus it's easier for muscle memory if the words are in the same order as javascripttoJson
vstoJSON
vstoJsonString
vstoJSONString
don't appeal to me.json::parse()
orjson.parse
, but I'm not sure these add much at this point.The meta problem here is that we'd like Hog functions to have as similar names to ClickHouse (& HogQL) functions as possible. CH doesn't have such JSON parsing functions, and we don't know if they'll ever add anything with these names. If they do, it'll be
prototype.js
all over again. It's probably fine though.How did you test this code?
Added tests