Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
PPL: Add
json_extract
function #3262base: main
Are you sure you want to change the base?
PPL: Add
json_extract
function #3262Changes from 84 commits
70152eb
76c3995
ce2c551
ad1bde3
ccf47a2
acc76a0
519c6f2
2e319fe
ee0820d
d44fc5a
3407d4a
7ef6cc9
e5e90ac
2187a5a
5e1e488
3512b33
9fea606
fbc54bc
31ad2a4
2b2a8f3
dc96563
1913bfe
67d979d
aa6b723
4c99235
9ccde7f
4306bf3
613137b
ab28872
3ec16e0
6dbf37b
b8c6d68
afb668c
54ef183
d841394
25fb527
4a20d08
fdc4729
707a0b9
4f28211
9ec6335
3324e66
7123c35
dbca991
7df87cb
cd45fcc
6f5dc07
0aae36e
b225f28
78af4f8
b84282a
1e23286
343f5a2
e8b6df3
ab9be75
788be9d
a9721bf
daa95ff
018e462
c6c6cc1
a5652ea
2cd10a2
cd78ddd
afb385f
0e91b2e
794db8a
0f0b8d4
f030057
2b08007
be52786
6bd2f40
0b9e9e4
6678be4
e57fa21
112be65
306ac97
75e9cc3
77827bb
80f44e2
0d1cc28
b6ae5ba
adde88d
aa8b81e
ec6ff5e
95e996b
591fb71
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Since know that the array list is always going to be the same since as
paths
, can we just initialize it with a specifiedinitialCapacity
to save some memory space?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.
resolved
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.
Looks like an errant debug statement?
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.
resolved
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.
Do we need a separate case for either of these? Seems like this would just throw a
PathNotFoundException
and returnsLITERAL_NULL
below if not (at least for "null", I'm not sure about the empty string). Probably makes sense to reduce branching here if possible. 🤷Moreover, do we actually want to return
LITERAL_NULL
for the empty string? Is that an invalid JSON string? If so, does it make sense to raise the corresponding exception instead?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.
both "null" and "" are considered valid json strings by json_valid, since we expect that users will use json_valid to first filter for valid json then use json extract to avoid errors, we need these specific cases to be consistent with json_valid
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.
Do we have tests for the following:
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.
added
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.
You could split
execute_extract_json
into two methods:execute_extract_json
(which would just actually runjsonExtract
) andtest_extract_json
(orassert_extract_json
), which would callexecute_extract_json
and then actually do the comparison.That way, test methods like this could call
execute_extract_json
, and no have to worry about callingDSL.literal
🤷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.
resolved
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.
Thanks!
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.
Can this be generalized to allow multiple paths, so you can use it for
json_extract_search_list_of_paths
as well?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.
resolved