Adds read/write support for v1.1 system symbols #847

zslayton · 2024-11-04T18:48:20Z

Adds static v1.1 system symbol table definition
Adds SystemSymbol_1_1 variant of RawSymbolRef
Renames old RawSymbolRef::as_raw_symbol_token_ref to shorter as_raw_symbol_ref for consistency.
Adds read/write support for 0xEE opcode, system symbol values
Adds read/write support for system symbols appearing in FlexSym
The application-level writer now uses system symbols for its encoding directives

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

zslayton

🗺️ PR tour 🧭

zslayton · 2024-11-04T18:51:30Z

src/lazy/any_encoding.rs

+    // Before a reader using `AnyEncoding` begins reading, it expects text Ion v1.0.
+    // At the outset of the stream, it inspects the first bytes to see if the stream is binary or text.
+    // If it encounters a version marker, the expected version will change.
+    const INITIAL_ENCODING_EXPECTED: IonEncoding = IonEncoding::Text_1_0;


🗺️ When a reader is initialized for a given Decoder, it needs to know what initial context to set up. For the four encoding-specific decoders, this is trivial; AnyEncoding is the only one that needs an explicit hint.

zslayton · 2024-11-04T18:52:01Z

src/lazy/any_encoding.rs

-            reader.next(context_ref).expect_err("Ion 1.1 IVM should return an error.");
-            return Ok(())
+            reader
+                .next(context_ref)
+                .expect_err("Ion 1.1 IVM should return an error.");
+            return Ok(());


🗺️ rustfmt, no logic change

zslayton · 2024-11-04T18:53:44Z

src/lazy/binary/raw/v1_1/immutable_buffer.rs

-    #[case::single_address(&[0xE4, 0x07], 1, 1)]
-    #[case::two_addresses(&[0xE5, 0x07, 0x09], 1, 2)]
-    #[case::three_addresses(&[0xE6, 0x07, 0x07, 0x09, 0x0B], 2, 3)]
-    #[case::single_flex_sym(&[0xE7, 0x07], 1, 1)]
-    #[case::two_flex_syms(&[0xE8, 0x07, 0x09], 1, 2)]
-    #[case::three_flex_syms(&[0xE9, 0x07, 0x07, 0x09, 0x0B], 2, 3)]


🗺️ Previously, this test was only confirming that the annotations sequence parts were the expected length after parsing. I augmented the test to include confirming that the annotations being read were the correct ones.

zslayton · 2024-11-04T18:55:00Z

src/lazy/binary/raw/v1_1/reader.rs

-        assert_eq!(
-            reader
-                .next(context)?
-                .expect_value()?
-                .read()?
-                .expect_symbol()?,
-            "".into()
-        );


🗺️ I rewrote this unit test to loop a list of expected symbols and compare them to the input. This made the test much more concise.

popematt · 2024-11-05T19:15:25Z

src/lazy/binary/raw/v1_1/value.rs

+                // In order to minimize the changes needed to introduce a second address space
+                // for symbols in Ion 1.1, system symbol IDs are resolved eagerly and returned
+                // as `Text`.


That's pretty much how I tried to do it when I added read support to ion-java.

popematt · 2024-11-05T19:21:03Z

src/lazy/encoder/binary/v1_0/container_writers.rs

+                RawSymbolRef::SystemSymbol_1_1(_system_symbol) => {
+                    unreachable!("Ion 1.1 symbol in Ion 1.0 writer")
+                }


Is it possible to make this provably unreachable with the use of Never or something like that, or would that add too much complexity in the way of generic parameters that are polluting signatures everywhere?

I think the best we could do without pervasive generics is have a separate enum with only SymbolId/Text and 'downgrade' to that whenever we enter a 1.0-only code path.

src/lazy/encoder/binary/v1_0/value_writer.rs

src/lazy/encoder/binary/v1_1/flex_sym.rs

popematt · 2024-11-05T19:39:09Z

src/raw_symbol_ref.rs

+    }
+}
+
+pub(crate) trait SystemSymbol: Sized {


Where is this trait used? I recall seeing SystemSymbol_1_1 used somewhere, but not this trait.

Initially I had a SystemSymbol struct that held an (IonVersion, SymbolAddress) tuple. SystemSymbolTable instances would return SystemSymbols, and RawSymbolRef had a SystemSymbol variant that wasn't Ion-1.1-specific.

However, this led to a lot of situations in Ion 1.0 code where it was unclear whether an Ion 1.0 system symbol would be represented as a SymbolId or a SystemSymbol. To eliminate that ambiguity, I split SystemSymbol into SystemSymbol_1_1 and SystemSymbol_1_0 and narrowed RawSymbolRef::SystemSymbol to RawSymbolRef::SystemSymbol_1_1. Then I turned the SystemSymbol type into a trait.

While I think this was a reasonable way to preserve the abstraction, we aren't using the abstraction much in practice right now. In the spirit of YAGNI, I can strip it out for the moment and we can reintroduce it later if/when we have a more concrete use case.

I'm not opposed to having it, necessarily, but if we're adding something that isn't used yet, we should probably document that in the code.

src/raw_symbol_ref.rs

src/text/text_formatter.rs

popematt · 2024-11-05T19:59:11Z

src/types/symbol.rs

@@ -98,6 +99,12 @@ impl Symbol {
        }
    }

+    pub fn system(text: &'static str) -> Symbol {


This is actually something that maybe useful for non-system symbols, right? For example, if I'm implementing WriteAsIon for Ion Schema Language models, there's a whole lot of symbol text that is statically known (i.e. all of the constraint names and other ISL keywords).

(Unfortunately, we can't update impl From<&str> for Symbol in such a way that it specializes for static and non-static strings, so use of a constructor function is required to specifically construct a symbol with static text.)

I originally named this method static but... that's a keyword :(

I renamed it static_text--howzat?

jobarr-amzn · 2024-11-05T19:40:10Z

src/constants.rs

+    /// A static, read-only map of text to Ion v1.0 system symbol addresses.
+    /// Because the set of string keys is known at compile time, this map is able to use a
+    /// perfect hashing function (PHF) to optimize lookup operations for those keys.
+    pub(crate) static SYSTEM_SYMBOL_TEXT_TO_ID: phf::Map<&str, usize> = phf_map! {


TIL https://docs.rs/phf/latest/phf/index.html

This is pretty neat!

src/lazy/encoder/writer.rs

src/symbol_table.rs

src/raw_symbol_ref.rs

jobarr-amzn · 2024-11-05T21:08:41Z

src/raw_symbol_ref.rs

+    /// Creates a new system symbol if the provided address is non-zero and in range for the
+    /// corresponding system symbol table.
+    fn new(symbol_address: SymbolAddress) -> Option<Self> {
+        match symbol_address {
+            0 => None,
+            address if address >= Self::system_symbol_table().len() => None,
+            address => Some(Self::new_unchecked(address)),
+        }
+    }
+
+    /// Constructs a system symbol from the provided address, returning `Ok(symbol)`.
+    /// If the provided address is zero or out of bounds, returns an `Err`.
+    #[inline]
+    fn try_new(symbol_address: SymbolAddress) -> IonResult<Self> {
+        Self::new(symbol_address).ok_or_else(|| {
+            ice!(IonError::decoding_error(format!(
+                "system symbol address {symbol_address} is out of bounds"
+            )))
+        })
+    }


From https://doc.rust-lang.org/nomicon/constructors.html

In concrete contexts, a type will provide a static new method for any kind of "default" constructor. This has no relation to new in other languages and has no special meaning. It's just a naming convention.

I was curious about the try_new here, wondered what I was missing. Apparently it's just a convention amongst libraries? I see the pattern in several others including in the standard library.

Also seen here: https://dev.to/cthutu/rust-5-naming-conventions-3cjf

Nothing to do here, just commenting.

Apparently it's just a convention amongst libraries?

Yep. Methods that return a Result<T> often have a name starting with try_ to distinguish themselves from sibling methods that return T or Option<T>. For example, into()/try_into() and from()/try_from().

Pulling a snippet from your link for others' convenience:

src/text/text_formatter.rs

zslayton added 8 commits October 24, 2024 17:06

defines 1.1 system symbols

2042f29

Adds read support for system symbols as values, annotations

042d714

Adds full read/write support for system symbols

1ac6b8a

Shortens as_raw_symbol_token_ref to as_raw_symbol_ref

336fff7

Adds inherent address method to SystemSymbol_1_1

f3f7007

Merge branch 'main' into system-symbols

d26c568

Fixes weird spacing.

2fa3577

Fixes more weird spacing.

cbc979b

zslayton commented Nov 4, 2024

View reviewed changes

zslayton requested review from popematt and jobarr-amzn November 4, 2024 19:02

clippy suggestions

1df352f

popematt reviewed Nov 5, 2024

View reviewed changes

jobarr-amzn reviewed Nov 5, 2024

View reviewed changes

zslayton added 4 commits November 6, 2024 09:39

Removes SystemSymbol trait and SystemSymbol_1_0 type

8d21e0a

incorporates review feedback

b09ef79

Puts use statements for 1.1 tests behind feature flag

36bf4cf

When a 1.0 writer gets a 1.1 token, err instead of panicking

4a0f545

zslayton requested review from jobarr-amzn and popematt November 6, 2024 16:18

popematt approved these changes Nov 6, 2024

View reviewed changes

src/text/text_formatter.rs Outdated Show resolved Hide resolved

jobarr-amzn approved these changes Nov 6, 2024

View reviewed changes

Remove early return

a7a15bc

zslayton merged commit 89d3206 into main Nov 6, 2024
32 checks passed

zslayton deleted the system-symbols branch November 6, 2024 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds read/write support for v1.1 system symbols #847

Adds read/write support for v1.1 system symbols #847

zslayton commented Nov 4, 2024

zslayton left a comment

zslayton Nov 4, 2024

zslayton Nov 4, 2024

zslayton Nov 4, 2024

zslayton Nov 4, 2024

popematt Nov 5, 2024

popematt Nov 5, 2024

zslayton Nov 6, 2024

popematt Nov 5, 2024

zslayton Nov 6, 2024

popematt Nov 6, 2024

popematt Nov 5, 2024

zslayton Nov 6, 2024

jobarr-amzn Nov 5, 2024

jobarr-amzn Nov 5, 2024

zslayton Nov 6, 2024

Adds read/write support for v1.1 system symbols #847

Adds read/write support for v1.1 system symbols #847

Conversation

zslayton commented Nov 4, 2024

zslayton 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment