Skip to content

Conversation

@zslayton
Copy link
Contributor

@zslayton zslayton commented Mar 18, 2025

Lays the groundwork for an ion jq command.

This PR adds a dependency on the jaq crate, which provides a jq engine, and implements the necessary traits for the Element type.

There are many todo!()s left to implement to support jq features like math operations. However, many basic commands work:

image

There are plenty of open questions around which jq flags to support, what the default expression should be, etc.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@zslayton zslayton requested a review from jobarr-amzn March 18, 2025 20:31
Copy link
Contributor

@jobarr-amzn jobarr-amzn left a comment

Choose a reason for hiding this comment

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

Amazing! Thanks for laying the groundwork here. What an excellent library jaq is. This is a great starter, and we can incrementally fill in the todos.

It is reading the whole stream into memory though, isn't it?

Comment on lines +112 to +118
let mut reader = Reader::new(AnyEncoding, input.into_source())?;
let input_elements = reader.read_all_elements()?;
let ion_stream_as_element = List::from(input_elements).into();

let inputs = RcIter::new(core::iter::empty());
// iterator over the output values
let out = filter.run((jaq_core::Ctx::new([], &inputs), ion_stream_as_element));
Copy link
Contributor

Choose a reason for hiding this comment

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

This could use some commentary as to what is happening. It's slurping the entire stream into memory and then feeding it through the filter, yeah?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, yeah. I followed one of (or the only?) example in their repo for the sake of getting this PR out. I think we can do something more efficient, but I don't know enough about filters' contract yet to be sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there is not presently a better way, see: 01mf02/jaq#276

.ok_or_else(|| jaq_err("index out of bounds"))?;
Ok(JaqElement::from(element.to_owned()))
}
// TODO: Should we allow indexing into a struct by integer?
Copy link
Contributor

Choose a reason for hiding this comment

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

Nope!

echo '{"foo": "bar"}' | jq '.[0]'
jq: error (at <stdin>:1): Cannot index object with number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for checking! I'm only somewhat familiar with jq's behavior, so there are lots of things left to double-check.

@jobarr-amzn
Copy link
Contributor

Fixes: #41

@zslayton zslayton merged commit 29bf0d0 into main Mar 31, 2025
4 checks passed
@zslayton zslayton deleted the ion-jq branch March 31, 2025 13:39
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