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

Promotes head, improves inspect #118

Merged
merged 6 commits into from
Jun 6, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ clap = { version = "4.0.17", features = ["cargo"] }
colored = "2.0.0"
flate2 = "1.0"
infer = "0.15.0"
ion-rs = { version = "1.0.0-rc.5", features = ["experimental"] }
ion-rs = { version = "1.0.0-rc.6", git = "https://github.com/amazon-ion/ion-rust.git", branch = "version-bump-rc6", features = ["experimental"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ This PR depends on amazon-ion/ion-rust#785. Once that's merged/released, I'll update this to point to crates.io.

tempfile = "3.2.0"
ion-schema = "0.10.0"
serde = { version = "1.0.163", features = ["derive"] }
Expand Down
3 changes: 0 additions & 3 deletions src/bin/ion/commands/beta/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ pub mod from;

#[cfg(feature = "experimental-code-gen")]
pub mod generate;
pub mod head;
pub mod primitive;
pub mod schema;
pub mod symtab;
Expand All @@ -13,7 +12,6 @@ use crate::commands::beta::count::CountCommand;
use crate::commands::beta::from::FromNamespace;
#[cfg(feature = "experimental-code-gen")]
use crate::commands::beta::generate::GenerateCommand;
use crate::commands::beta::head::HeadCommand;
use crate::commands::beta::primitive::PrimitiveCommand;
use crate::commands::beta::schema::SchemaNamespace;
use crate::commands::beta::symtab::SymtabNamespace;
Expand All @@ -36,7 +34,6 @@ impl IonCliCommand for BetaNamespace {
Box::new(CountCommand),
Box::new(PrimitiveCommand),
Box::new(SchemaNamespace),
Box::new(HeadCommand),
Box::new(FromNamespace),
Box::new(ToNamespace),
Box::new(SymtabNamespace),
Expand Down
1 change: 0 additions & 1 deletion src/bin/ion/commands/beta/symtab/filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ impl IonCliCommand for SymtabFilterCommand {
fn configure_args(&self, command: Command) -> Command {
command.with_input()
.with_output()
.with_compression_control()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ Prior to this PR, commands needed to explicitly opt into supporting the --no-auto-decompress flag. Forgetting to do so would cause the program to ugly-crash when the flag was specified. Now the flag is supported universally by default.

.arg(Arg::new("lift")
.long("lift")
.short('l')
Expand Down
5 changes: 1 addition & 4 deletions src/bin/ion/commands/beta/to/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,7 @@ impl IonCliCommand for ToJsonCommand {
fn configure_args(&self, command: Command) -> Command {
// NOTE: it may be necessary to add format-specific options. For example, a "pretty" option
// would make sense for JSON, but not binary formats like CBOR.
command
.with_input()
.with_output()
.with_compression_control()
command.with_input().with_output()
}

fn run(&self, _command_path: &mut Vec<String>, args: &ArgMatches) -> Result<()> {
Expand Down
6 changes: 1 addition & 5 deletions src/bin/ion/commands/cat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,7 @@ impl IonCliCommand for CatCommand {
}

fn configure_args(&self, command: Command) -> Command {
command
.with_input()
.with_output()
.with_format()
.with_compression_control()
command.with_input().with_output().with_format()
}

fn run(&self, _command_path: &mut Vec<String>, args: &ArgMatches) -> Result<()> {
Expand Down
File renamed without changes.
70 changes: 39 additions & 31 deletions src/bin/ion/commands/inspect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
use std::io::Write;
use std::str::FromStr;

use anyhow::{Context, Result};
use anyhow::{bail, Context, Result};
use clap::{Arg, ArgMatches, Command};
use ion_rs::v1_0::{LazyRawBinaryValue, RawValueRef};
use ion_rs::*;
Expand Down Expand Up @@ -187,22 +187,39 @@
Ok(inspector)
}

fn confirm_encoding_is_supported(&self, encoding: IonEncoding) -> Result<()> {
use IonEncoding::*;
match encoding {
Text_1_0 | Text_1_1 => {
bail!("`inspect` does not support text Ion streams.");
}
Binary_1_0 => Ok(()),
Binary_1_1 => bail!("`inspect` does not yet support binary Ion v1.1"),
// `IonEncoding` is #[non_exhaustive]
_ => bail!("`inspect does not yet support {}", encoding.name()),
}
}
Comment on lines +190 to +201
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ The latest ion_rs makes it possible to get the IonEncoding from any raw stream item. This allows us to trivially confirm that we're working with what we expect; in inspect's case, that's binary Ion 1.0.


/// Iterates over the items in `reader`, printing a table section for each top level value.
fn inspect_top_level<Input: IonInput>(
&mut self,
reader: &mut SystemReader<AnyEncoding, Input>,
) -> Result<()> {
const TOP_LEVEL_DEPTH: usize = 0;
self.write_table_header()?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ We now defer writing the table header until after we've read the first item from the stream and confirmed it's of a supported encoding.


let mut is_first_item = true;
let mut has_printed_skip_message = false;
loop {
// TODO: This does not account for shared symbol table imports. However, the CLI does not
// yet support specifying a catalog, so it's correct enough for the moment.
let mut next_symbol_id = reader.symbol_table().len();
let item = reader.next_item()?;
let is_last_item = matches!(item, SystemStreamItem::EndOfStream(_));

if is_first_item {
// The first item in a stream cannot be ephemeral, so we can safely unwrap this.
let encoding = item.raw_stream_item().unwrap().encoding();
self.confirm_encoding_is_supported(encoding)?;
self.write_table_header()?;
}

match self.select_action(
TOP_LEVEL_DEPTH,
&mut has_printed_skip_message,
Expand All @@ -222,17 +239,13 @@

match item {
SystemStreamItem::SymbolTable(lazy_struct) => {
let is_append = lazy_struct.get("imports")?
== Some(ValueRef::Symbol(SymbolRef::with_text("$ion_symbol_table")));
if !is_append {
next_symbol_id = 10; // First available SID after system symbols in Ion 1.0
}
self.inspect_symbol_table(next_symbol_id, lazy_struct)?;
Comment on lines -225 to -230
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ Now that a LazyValue can return a reference to its SymbolTable, the symbol ID assignment logic can be localized to the method where it's displayed.

self.inspect_symbol_table(lazy_struct)?;
}
SystemStreamItem::Value(lazy_value) => {
self.inspect_value(0, "", lazy_value, no_comment())?;
}
SystemStreamItem::VersionMarker(marker) => {
self.confirm_encoding_is_supported(marker.encoding())?;
self.inspect_ivm(marker)?;
}
SystemStreamItem::EndOfStream(_) => {
Expand Down Expand Up @@ -462,11 +475,7 @@
}
}

fn inspect_symbol_table(
&mut self,
next_symbol_id: usize,
struct_: LazyStruct<'_, AnyEncoding>,
) -> Result<()> {
fn inspect_symbol_table(&mut self, struct_: LazyStruct<'_, AnyEncoding>) -> Result<()> {
let value = struct_.as_value();
if value.has_annotations() {
self.newline()?;
Expand All @@ -479,9 +488,7 @@

use LazyRawValueKind::*;
match raw_struct.as_value().kind() {
Binary_1_0(v) => {
self.inspect_binary_1_0_symbol_table(next_symbol_id, struct_, raw_struct, v)
}
Binary_1_0(v) => self.inspect_binary_1_0_symbol_table(struct_, raw_struct, v),
Binary_1_1(_) => todo!("Binary Ion 1.1 symbol table"),
Text_1_0(_) | Text_1_1(_) => unreachable!("text value"),
}
Expand Down Expand Up @@ -742,7 +749,6 @@

fn inspect_binary_1_0_symbol_table(
&mut self,
next_symbol_id: usize,
struct_: LazyStruct<AnyEncoding>,
raw_struct: LazyRawAnyStruct,
raw_value: LazyRawBinaryValue,
Expand Down Expand Up @@ -771,7 +777,7 @@
)? {
InspectorAction::Skip => continue,
InspectorAction::Inspect if field.name()? == "symbols" => {
self.inspect_lst_symbols_field(next_symbol_id, field, raw_field)?
self.inspect_lst_symbols_field(struct_, field, raw_field)?
}
// TODO: if field.name()? == "imports" => {}
InspectorAction::Inspect => {
Expand All @@ -791,7 +797,7 @@

fn inspect_lst_symbols_field(
&mut self,
mut next_symbol_id: usize,
symtab_struct: LazyStruct<AnyEncoding>,
field: LazyField<AnyEncoding>,
raw_field: LazyRawFieldExpr<AnyEncoding>,
) -> Result<()> {
Expand Down Expand Up @@ -825,6 +831,16 @@
Ok(())
})?;

// TODO: This does not account for shared symbol table imports. However, the CLI does not
// yet support specifying a catalog, so it's correct enough for the moment.
let symtab_value = symtab_struct.as_value();
let mut next_symbol_id = symtab_value.symbol_table().len();

Check failure on line 837 in src/bin/ion/commands/inspect.rs

View workflow job for this annotation

GitHub Actions / Build and Test (macos-latest)

`symtab_value` does not live long enough

Check failure on line 837 in src/bin/ion/commands/inspect.rs

View workflow job for this annotation

GitHub Actions / Build and Test (ubuntu-latest)

`symtab_value` does not live long enough

Check failure on line 837 in src/bin/ion/commands/inspect.rs

View workflow job for this annotation

GitHub Actions / Build and Test (macos-latest)

`symtab_value` does not live long enough
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ We don't have to track which ID we were on because we can ask the symbol table what the next ID will be.

let is_append = symtab_struct.get("imports")?
== Some(ValueRef::Symbol(SymbolRef::with_text("$ion_symbol_table")));
if !is_append {
next_symbol_id = 10; // First available SID after system symbols in Ion 1.0
}

let mut has_printed_skip_message = false;
for (raw_value_res, value_res) in nested_raw_values.zip(nested_values) {
let (raw_nested_value, nested_value) = (raw_value_res?, value_res?);
Expand Down Expand Up @@ -925,15 +941,7 @@
BytesKind::TrailingLength,
encoding.trailing_length_span().bytes(),
);
// TODO: There is a bug in the `body_span()` method that causes it fail when the value is annotated.
// When it's fixed, this can be:
// let body_bytes = IonBytes::new(BytesKind::ValueBody, body_span);
let body_len = raw_value.encoded_data().body_range().len();
let total_len = raw_value.encoded_data().range().len();
let body_bytes = IonBytes::new(
BytesKind::ValueBody,
&encoding.span().bytes()[total_len - body_len..],
);
let body_bytes = IonBytes::new(BytesKind::ValueBody, encoding.body_span().bytes());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ ion_rs fixed the bug in body_span(), so I was able to simplify this.


let mut formatter =
BytesFormatter::new(BYTES_PER_ROW, vec![opcode_bytes, length_bytes, body_bytes]);
Expand Down
8 changes: 5 additions & 3 deletions src/bin/ion/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use termcolor::{ColorChoice, StandardStream, StandardStreamLock};
pub mod beta;
pub mod cat;
pub mod dump;
pub mod head;
pub mod inspect;

/// Behaviors common to all Ion CLI commands, including both namespaces (groups of commands)
Expand Down Expand Up @@ -51,7 +52,8 @@ pub trait IonCliCommand {
.about(self.about())
.version(crate_version!())
.author(crate_authors!())
.hide(self.hide_from_help_message());
.hide(self.hide_from_help_message())
.with_compression_control();

// If there are subcommands, add them to the configuration and set 'subcommand_required'.
if !clap_subcommands.is_empty() {
Expand Down Expand Up @@ -175,8 +177,8 @@ impl<'a> CommandIo<'a> {

/// Returns `true` if the user has not explicitly disabled auto decompression.
fn auto_decompression_enabled(&self) -> bool {
if let Some(flag) = self.args.get_one::<bool>("no-auto-decompress") {
!*flag
if let Some(is_disabled) = self.args.get_one::<bool>("no-auto-decompress") {
!*is_disabled
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ This is just a naming change for clarity.

} else {
true
}
Expand Down
4 changes: 3 additions & 1 deletion src/bin/ion/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use ion_rs::IonError;
use std::io::ErrorKind;

use crate::commands::dump::DumpCommand;
use crate::commands::head::HeadCommand;
use crate::commands::inspect::InspectCommand;

fn main() -> Result<()> {
Expand Down Expand Up @@ -48,8 +49,9 @@ impl IonCliCommand for RootCommand {
fn subcommands(&self) -> Vec<Box<dyn IonCliCommand>> {
vec![
Box::new(BetaNamespace),
Box::new(DumpCommand),
Box::new(CatCommand),
Box::new(DumpCommand),
Box::new(HeadCommand),
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 put the commands in alphabetical order while I was here.

Box::new(InspectCommand),
]
}
Expand Down
1 change: 0 additions & 1 deletion tests/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,6 @@ fn test_write_all_values(#[case] number: i32, #[case] expected_output: &str) ->
input_file.write_all(test_data.as_bytes())?;
input_file.flush()?;
cmd.args([
"beta",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ This unit test invoked ion beta head and needed to be updated to ion head.

"head",
"--values",
&number.to_string(),
Expand Down
Loading