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

add customer functions for jmespath #560

Merged
merged 3 commits into from
Nov 26, 2024

Conversation

Uniplore-X
Copy link
Contributor

  1. fix a bug of avg function.
  2. add customer functions to enhance the processing capabilities of jmespath extension.(I just tested the example on Ubuntu 22.04.)
  3. fix a bug of jmespath test (function expression "not_null()" with no args, should result null instead of invalid-arity error)

…be an average value returned instead of a null value.
…result null instead of invalid-arity error (after fix bug for no-args-function)
@danielaparker danielaparker merged commit 04a3ceb into danielaparker:master Nov 26, 2024
54 checks passed
@danielaparker
Copy link
Owner

Thanks for contributing!

@danielaparker
Copy link
Owner

danielaparker commented Dec 11, 2024

Just a follow up to your customer functions contribution. I was impressed that you were able to do that given that the essential types you needed were deeply buried as inner classes all inside a detail namespace. I thought your example was quite good too.

Regarding this code and comment

// result->emplace_back(ele); // ?: It may lead to an abnormal exit.
result->emplace_back(*resources.create_json(deep_copy(ele)));

the issue is with these lines

const auto context = args[0].value();   (*)
// ...
auto ele = expr.evaluate(context, resources, ec2);

The first line should have been

const auto& context = args[0].value();   (**)

evaluate expects its reference argument to live through the entire evaluation, but here context dies when the function returns. If written as (**)

result->emplace_back(ele);

is fine, as would be (how it would be done internally by evaluate, to minimize copies)

const auto& ele = expr.evaluate(context, resources, ec2);
result->emplace_back(json_const_pointer_arg, &ele);

Looking at the code, I see a lot of *resources.create_json() and std::addressof. To eliminate most of that, I changed evaluate (including function evaluate) to return a const Json* pointer rather than a const Json& reference. I also renamed null_value(), true_value(), false_value() and create_json() to make_null(), make_true, make_false, make_json and have them consistently return a pointer to a Json value.

I also moved classes parameter and dynamic_resources out of inner classes and detail namespace, and restructured the code to be as close as possible to custom functions in jsonpath. The current version of the example is here.

Any feedback appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants