Skip to content
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(expr): support jsonb_extract_path(_text) function #13143

Merged
merged 6 commits into from Oct 31, 2023

Conversation

wangrunji0408
Copy link
Contributor

@wangrunji0408 wangrunji0408 commented Oct 30, 2023

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Based on #13110, this PR implements their variadic form functions:

jsonb_extract_path ( from_json jsonb, VARIADIC path_elems text[] ) → jsonb
jsonb_extract_path_text ( from_json jsonb, VARIADIC path_elems text[] ) → text

by rewriting variadic arguments to array[] in the frontend. e.g.

jsonb_extract_path(jsonb, s1, s2) => jsonb_extract_path(jsonb, array[s1, s2])

It seems that variadic functions still need special handlings in type inference. We might consider to support such signatures and automate rewrite in the future:

#[function("jsonb_extract_path(jsonb, variadic varchar[]) -> jsonb")]

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)
jsonb_extract_path ( from_json jsonb, VARIADIC path_elems text[] ) → jsonb
Extracts JSON sub-object at the specified path. (This is functionally equivalent to the #> operator, but writing the path out as a variadic list can be more convenient in some cases.)
json_extract_path('{"f2":{"f3":1},"f4":{"f5":99,"f6":"foo"}}', 'f4', 'f6') → "foo"

jsonb_extract_path_text ( from_json jsonb, VARIADIC path_elems text[] ) → text
Extracts JSON sub-object at the specified path as text. (This is functionally equivalent to the #>> operator.)
json_extract_path_text('{"f2":{"f3":1},"f4":{"f5":99,"f6":"foo"}}', 'f4', 'f6') → foo

Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Merging #13143 (4908375) into main (c97e08c) will decrease coverage by 0.01%.
The diff coverage is 48.64%.

@@            Coverage Diff             @@
##             main   #13143      +/-   ##
==========================================
- Coverage   68.26%   68.26%   -0.01%     
==========================================
  Files        1505     1505              
  Lines      254901   254931      +30     
==========================================
+ Hits       174015   174032      +17     
- Misses      80886    80899      +13     
Flag Coverage Δ
rust 68.26% <48.64%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/frontend/src/expr/pure.rs 87.69% <ø> (ø)
src/frontend/src/binder/expr/binary_op.rs 70.40% <0.00%> (ø)
src/expr/impl/src/scalar/jsonb_access.rs 0.00% <0.00%> (ø)
src/frontend/src/binder/expr/function.rs 78.28% <60.00%> (-0.47%) ⬇️

... and 8 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@KeXiangWang KeXiangWang left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Runji Wang <wangrunji0408@163.com>
@xiangjinwu

This comment was marked as outdated.

@xiangjinwu
Copy link
Contributor

xiangjinwu commented Oct 31, 2023

Sorry for the previous comment. After checking deeper, the PostgreSQL way is:

  • greatest/coalesce are not in pg_proc at all and handled specially.
  • concat/format are true variadic in doc. Their signature was not declared as variadic any[] but any, ..., because any[] is an invalid thing. This means it can be invoked with concat(1, true) or concat(variadic array['1', 'true']) but not concat(variadic array[1, true]). Its backend implementation has different paths handling variadic array and any, ...
  • jsonb_extract_path is declared as variadic text[], supports jsonb_extract_path('["a", {"b":"c"}]'::jsonb, '1', 'b') and jsonb_extract_path('["a", {"b":"c"}]'::jsonb, '1', 'b') but not jsonb_extract_path('["a", {"b":"c"}]'::jsonb, 1, 'b'). Its backend implementation only handles array. However I have not confirmed whether PostgreSQL transformed the args into an array. confirmed

Now back to our implementation:

  • impl Row for ListRef means we accept an array by expanding it to an implementation that handles , ... which is the reverse of what PostgreSQL did. I do not have a strong option on which is better, just want to bring it up it is also possible to build a literal array from , ... args in frontend.

@wangrunji0408
Copy link
Contributor Author

It is better that jsonb_extract_path use the same array-based backend implementation as #>.

I see. So it turns out that jsonb_extract_path is the identical function of the #> operator and this PR should be rewritten to a pure frontend handling.😮

@xiangjinwu
Copy link
Contributor

xiangjinwu commented Oct 31, 2023

It is better that jsonb_extract_path use the same array-based backend implementation as #>.

I see. So it turns out that jsonb_extract_path is the identical function of the #> operator and this PR should be rewritten to a pure frontend handling.😮

Yes, PostgreSQL resolves #> to the jsonb_extract_path function. (This fact led me to think other variadic-like functions greatest or concat are similar in frontend syntax and backend implementation, but my first comment only checked their frontend syntax and the second comment is after checking their backend implementation.)

test=# select oprcode from pg_operator where oprname = '#>';
      oprcode       
--------------------
 json_extract_path
 jsonb_extract_path
(2 rows)

Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
@wangrunji0408 wangrunji0408 added this pull request to the merge queue Oct 31, 2023
Merged via the queue into main with commit 8a90ce5 Oct 31, 2023
28 of 29 checks passed
@wangrunji0408 wangrunji0408 deleted the wrj/jsonb-extract-path branch October 31, 2023 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants