Skip to content

Conversation

@jsuereth
Copy link
Contributor

@jsuereth jsuereth commented Dec 14, 2024

  • Update Filter to only remember strings, as we can't safely precompile with new lifetime shenanigans.
  • Create new jq file to hide all details of calling JQ
  • Update filter/engine to start passing serde-JSON structures by default, instead of JAQ internals.
  • Create error-message generation.
  • Update tests.

- Update Filter to only remember strings, as we can't safely precompile with new lifetime shenanigans.
- Create new jq file to hide all details of calling JQ
- Update filter/engine to start passing serde-JSON structures by default, instead of JAQ internals.
- Update tests.

Note: error messages are horrendous with this update.
@jsuereth jsuereth marked this pull request as ready for review December 16, 2024 16:00
@jsuereth jsuereth requested a review from a team as a code owner December 16, 2024 16:00
@jsuereth jsuereth changed the title Initial cut at moving to JAQ 2.0. Moving to JAQ 2.0 Dec 16, 2024
})?;

let (names, values) = prepare_jq_context(params);
let funs = jaq_std::funs().chain(jaq_json::funs());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I’m not entirely sure, but you could try something like this to make the intent more explicit.

fn adjust_lifetime<'a, T: 'a>(iter: impl Iterator<Item = T>) -> impl Iterator<Item = T> + 'a {
    iter
}
 
let funs = adjust_lifetime(jaq_std::funs().chain(jaq_json::funs()));
...
  .with_funs(funs)
  .compile(modules)
  ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately, that function doesn't work (iter may not live for lifetime of 'a). What we need is to trick the compiler to infer a narrower lifetime when seeing both 'a and 'static to return 'a, and we can only do that when they are in contravariant (or return-value) position. I haven't figured out how to do that yet, but I can look that up for future cleanup. I figure it's not important enough to block this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So i think I have my arrows backwards (using contravariant to mean covariant), but otherwise (while I didn't get a chance to fully read this), the answer probably lies here: https://doc.rust-lang.org/nomicon/subtyping.html

From what I can tell the variance rules are only used to change lifetimes within rust, the trick I'm using with lambda is the only way I could think of.

Copy link
Contributor

@lquerel lquerel left a comment

Choose a reason for hiding this comment

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

LGTM. Just two nit comments. Thank you, that was a significant effort to transition to this new JAQ version.

Comment on lines -24 to -26
ctx.insert_natives(jaq_core::core());
ctx.insert_defs(jaq_std::std());
ctx.insert_defs(defs);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It's probably ok but are we still supporting the same JQ core functions and definitions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we are. I'm pulling in the jaq_std defa and json defa

@jsuereth jsuereth enabled auto-merge (squash) December 17, 2024 03:45
@jsuereth jsuereth merged commit 3287506 into open-telemetry:main Dec 17, 2024
20 checks passed
@jsuereth jsuereth deleted the wip-bump-jaq-to-2.0 branch December 17, 2024 03:50
@codecov
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 8 lines in your changes missing coverage. Please review.

Project coverage is 74.5%. Comparing base (deca2e9) to head (a2c4925).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/weaver_forge/src/jq.rs 90.3% 6 Missing ⚠️
crates/weaver_forge/src/lib.rs 75.0% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #524     +/-   ##
=======================================
+ Coverage   74.1%   74.5%   +0.4%     
=======================================
  Files         50      51      +1     
  Lines       3946    3960     +14     
=======================================
+ Hits        2926    2954     +28     
+ Misses      1020    1006     -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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