Upgrade to `ion_rs` dependency to v1.0.0-rc.5 #113

zslayton · 2024-06-02T00:58:07Z

Upgrades the ion-cli's dependency on ion_rs from v1.0.0-rc.2 to v1.0.0-rc.5.
Removes our dependency on memmap; all commands are now streaming.
Removes hack to add newlines between per-file outputs from dump.
Overhauls the inspect command to take advantage of new tooling access APIs in the streaming reader. The new version:
- shows opcodes and VarUInt lengths in a style that is distinct from that of the body of the value
- uses Unicode box drawing to render its table
- colors annotations and field names in the text Ion column
- shows the symbol ID assigned to each string in a local symbol

Fixes issues #34 and #36.

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

popematt · 2024-06-03T17:16:24Z

src/bin/ion/commands/beta/symtab/filter.rs

+                let Some(raw_value) = symtab.as_value().raw() else {
+                    // This symbol table came from a macro expansion; there are no encoded bytes
+                    // to pass through.
+                    bail!("found an ephemeral symbol table, which is not yet supported")


Is "ephemeral" going to be an official-ish term for this case?

That's the phrase I've been using for it, but admittedly mostly to myself :)

If it resonates with you, then I'd say let's do it. 👍

zslayton

🗺️ PR tour

zslayton · 2024-06-03T16:51:08Z

src/bin/ion/commands/beta/inspect.rs

                        "When specified, the inspector will skip ahead `n` bytes before
-beginning to display the contents of the stream. System values like
-Ion version markers and symbol tables in the bytes being skipped will
-still be displayed. If the requested number of bytes falls in the
-middle of a value, the whole value (complete with field ID and
-annotations if applicable) will be displayed. If the value is nested
-in one or more containers, those containers will be displayed too.",
+beginning to display the contents of the stream. If the requested number
+of bytes falls in the middle of a scalar, the whole value (complete with
+field ID and annotations if applicable) will be displayed. If the value
+is nested in one or more containers, the opening delimiters of those
+containers be displayed.",


🗺️ Here's the inspect output for a simple data stream, and the output for the same stream with --skip-bytes 29 specified:

Notice that the ends of the containers that byte 29 falls within are still displayed for context.

zslayton · 2024-06-03T16:54:09Z

src/bin/ion/commands/beta/inspect.rs

                        "When specified, the inspector will stop printing values after
-processing `n` bytes of Ion data. If `n` falls within a value, the
-complete value will be displayed.",
+processing `n` bytes of Ion data. If `n` falls within a scalar, the
+complete value will be displayed. If `n` falls within one or more containers,
+the closing delimiters for those containers will be displayed. If this flag
+is used with `--skip-bytes`, `n` is counted from the beginning of the first
+value start after `--skip-bytes`.
+",


🗺️ Here's the inspect output for a simple data stream, and the output for the same stream with --limit-bytes 29 specified:

Notice that the starts of the containers that byte 29 falls within are still displayed for context.

zslayton · 2024-06-03T17:47:48Z

src/bin/ion/commands/beta/symtab/filter.rs

+                let Some(raw_value) = symtab.as_value().raw() else {
+                    // This symbol table came from a macro expansion; there are no encoded bytes
+                    // to pass through.
+                    bail!("found an ephemeral symbol table, which is not yet supported")


That's the phrase I've been using for it, but admittedly mostly to myself :)

If it resonates with you, then I'd say let's do it. 👍

zslayton · 2024-06-03T17:49:10Z

tests/cli.rs

 #[cfg(feature = "experimental-code-gen")]
-#[rstest]
-#[case::simple_struct(
-    r#"
+mod code_gen_tests {


🗺️ All of the changes in this file come from introducing a new module that could be more easily feature gated behind experimental-code-gen. There are no logic changes.

zslayton · 2024-06-03T17:49:41Z

tests/code-gen-tests.rs

@@ -1,12 +1,12 @@
+#![cfg(feature = "experimental-code-gen")]


🗺️ Similarly, I moved the feature gates in this file to the module level to avoid needing to repeat them.

jobarr-amzn

LGTM

src/bin/ion/commands/beta/symtab/filter.rs

jobarr-amzn · 2024-06-03T17:10:01Z

src/bin/ion/commands/dump.rs

-                if reader.is_null() {
-                    writer.write_null(ion_type)?;
-                    continue;
-                }
-
-                use IonType::*;
-                match ion_type {
-                    Null => unreachable!("null values are handled prior to this match"),
-                    Bool => writer.write_bool(reader.read_bool()?)?,
-                    Int => writer.write_int(&reader.read_int()?)?,
-                    Float => {
-                        let float64 = reader.read_f64()?;
-                        let float32 = float64 as f32;
-                        if float32 as f64 == float64 {
-                            // No data lost during cast; write it as an f32
-                            writer.write_f32(float32)?;
-                        } else {
-                            writer.write_f64(float64)?;
-                        }
-                    }
-                    Decimal => writer.write_decimal(&reader.read_decimal()?)?,
-                    Timestamp => writer.write_timestamp(&reader.read_timestamp()?)?,
-                    Symbol => writer.write_symbol(reader.read_symbol()?)?,
-                    String => writer.write_string(reader.read_string()?)?,
-                    Clob => writer.write_clob(reader.read_clob()?)?,
-                    Blob => writer.write_blob(reader.read_blob()?)?,
-                    List => {
-                        reader.step_in()?;
-                        writer.step_in(List)?;
-                    }
-                    SExp => {
-                        reader.step_in()?;
-                        writer.step_in(SExp)?;
-                    }
-                    Struct => {
-                        reader.step_in()?;
-                        writer.step_in(Struct)?;
-                    }
-                }
-            }
-            StreamItem::Nothing if reader.depth() > 0 => {
-                reader.step_out()?;
-                writer.step_out()?;
-            }
-            StreamItem::Nothing => break,
-        }
-        if reader.depth() == 0 {


Nice. What a good change. Way to go ion-rs!

src/bin/ion/commands/dump.rs

src/bin/ion/commands/mod.rs

jobarr-amzn · 2024-06-03T17:29:16Z

src/bin/ion/commands/beta/inspect.rs

+            let stdin_lock = io::stdin().lock();
            // If no input file was specified, run the inspector on STDIN.
-
-            // The inspector expects its input to be a byte array or mmap()ed file acting as a byte
-            // array. If the user wishes to provide data on STDIN, we'll need to copy those bytes to
-            // a temporary file and then read from that.
-
-            // Create a temporary file that will delete itself when the program ends.
-            let mut input_file = tempfile::tempfile().with_context(|| {
-                concat!(
-                    "Failed to create a temporary file to store STDIN.",
-                    "Try passing an --input flag instead."
-                )
-            })?;
-
-            // Pipe the data from STDIN to the temporary file.
-            let mut writer = BufWriter::new(input_file);
-            io::copy(&mut io::stdin(), &mut writer)
-                .with_context(|| "Failed to copy STDIN to a temp file.")?;
-            // Get our file handle back from the BufWriter
-            input_file = writer
-                .into_inner()
-                .with_context(|| "Failed to read from temp file containing STDIN data.")?;
-            // Read from the now-populated temporary file.
-            inspect_file(
-                "STDIN temp file",
-                input_file,
-                &mut output,
-                bytes_to_skip,
-                limit_bytes,
-            )?;
+            inspect_input("STDIN", stdin_lock, &mut output, bytes_to_skip, limit_bytes)?;


jobarr-amzn · 2024-06-03T17:49:50Z

src/bin/ion/commands/beta/inspect.rs

+    /// Convenience method to set the output stream to the specified color/style for the duration of `write_fn`
+    /// and then reset it upon completion.
+    fn with_style(
+        &mut self,
+        style: ColorSpec,
+        write_fn: impl FnOnce(&mut OutputRef) -> Result<()>,
+    ) -> Result<()> {
+        self.output.set_color(&style)?;
+        write_fn(&mut self.output)?;
+        self.output.reset()?;
+        Ok(())
    }


I'm a little surprised this doesn't already exist in termcolor, since this is usually the idiom I reach for for colorized text.
Indeed, though, it's not there: https://docs.rs/termcolor/latest/termcolor/trait.WriteColor.html

zslayton changed the title ~~Upgrade to rc4~~ Upgrade to rc5 Jun 3, 2024

zslayton changed the title ~~Upgrade to rc5~~ Upgrade to ion_rs dependency to v1.0.0-rc.5 Jun 3, 2024

zslayton added 9 commits June 3, 2024 12:33

Adds new ion beta inspect

a448c88

updates the dump and filter commands

2cf1a0e

update to json

d4c6173

Remove references to old version of ion_rs

1cc0d66

Refactoring to DRY things up

1d73dc2

inspect now shows symbol IDs assigned to LST symbols

e07f24f

Prints SID used to encode Symbol values.

73e3c91

Removes dependency on memmap

592c27f

formatting cleanup, set ion_rs dep to v1.0.0-rc.5

e59676d

zslayton force-pushed the upgrade-to-rc4 branch from 4a20877 to e59676d Compare June 3, 2024 16:43

Remove redundant newline after sequence start

235d3e7

popematt approved these changes Jun 3, 2024

View reviewed changes

removed old comment

a0d945f

zslayton commented Jun 3, 2024

View reviewed changes

cargo fmt

0bfeb24

jobarr-amzn approved these changes Jun 3, 2024

View reviewed changes

Removed outdated comment

f49c4cd

zslayton merged commit 19e5ea9 into master Jun 3, 2024
7 checks passed

zslayton deleted the upgrade-to-rc4 branch June 3, 2024 18:05

This was referenced Jun 3, 2024

inspect should also show symbol numbers in symbol tables #34

Closed

Iteratively parse top-level values in inspect and to #47

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to `ion_rs` dependency to v1.0.0-rc.5 #113

Upgrade to `ion_rs` dependency to v1.0.0-rc.5 #113

zslayton commented Jun 2, 2024 •

edited

Loading

popematt Jun 3, 2024

zslayton Jun 3, 2024

zslayton left a comment

zslayton Jun 3, 2024 •

edited

Loading

zslayton Jun 3, 2024

zslayton Jun 3, 2024

zslayton Jun 3, 2024

zslayton Jun 3, 2024

jobarr-amzn left a comment

jobarr-amzn Jun 3, 2024

jobarr-amzn Jun 3, 2024

jobarr-amzn Jun 3, 2024

Upgrade to ion_rs dependency to v1.0.0-rc.5 #113

Upgrade to ion_rs dependency to v1.0.0-rc.5 #113

Conversation

zslayton commented Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zslayton left a comment

Choose a reason for hiding this comment

zslayton Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jobarr-amzn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Upgrade to `ion_rs` dependency to v1.0.0-rc.5 #113

Upgrade to `ion_rs` dependency to v1.0.0-rc.5 #113

zslayton commented Jun 2, 2024 •

edited

Loading

zslayton Jun 3, 2024 •

edited

Loading