-
Notifications
You must be signed in to change notification settings - Fork 4
feat: Json type optimization #2006
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
Conversation
renderer := column.NewRenderer() | ||
|
||
for i := 0; i < b.N; i++ { | ||
// reqCtx needs to be created separately for each request, otherwise the parsed json is cached | ||
reqCtx := recordctx.FromHTTP(time.Now(), &http.Request{Header: header, Body: io.NopCloser(strings.NewReader(body))}) |
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.
reqCtx
caches the parsed json in case it is needed for other columns so we need to create a new one for each CSVValue
call, otherwise json parsing is cached, making the benchmark irrelevant.
4f03b80
to
20c8459
Compare
@@ -302,6 +302,154 @@ func TestRenderer_Path_FormData_Full(t *testing.T) { | |||
assert.Equal(t, `{"key1":"bar1","key2":["bar2","bar3"]}`, val) | |||
} | |||
|
|||
func TestRenderer_Path_FormData_Scalar(t *testing.T) { |
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.
Extra tests for application/x-www-form-urlencoded
since the original renderer implementation is no longer covered by json tests + the results changed because of ParseQuery
improvements.
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.
LGTM
87f54d9
to
514f841
Compare
67159a4
to
b158a2e
Compare
7e5e06a
to
8604191
Compare
Jira: https://keboola.atlassian.net/browse/PSGO-734
Changes:
Benchmarks
The
BenchmarkColumn_Path
benchmark is the relevant one here.Old implementation:
New implementation with
fastjson
: