Update conformance testing DSL to support unknown symbols #833

nirosys · 2024-09-09T10:15:33Z

Issue #, if available: n/a

Description of changes:
This PR adds missing symbol notation for Toplevel and Produces clauses.

This turned out to be a lot simpler than I had originally thought. My first (few) pass(es) through the spec left my understanding to be that the notation could define symbol mappings, which would have to be created during the evaluation of the fragments, and supported when evaluating expectations. Unfortunately I didn't realize my misunderstanding until after having implemented creating the symbol mappings.

The functionality now allows symbol IDs to be specified as quoted symbols, with the #$ notation for identifying such symbol IDs.

Details

When the Toplevel fragment is triggered to encode it's contents into a new document fragment, it uses ProxyElement to wrap each of the ion elements that were defined within its body. ProxyElement implements WriteAsIon in order to hijack any symbols being written and detect any #$ prefixed symbols so that it can parse out the symbol table name, and offset. Once parsed, it will write the symbol ID to the document fragment.

When a Produces clause is evaluated, it will walk through its body of ion elements, while reading the concatenated ion document. As each element is evaluated for equality, any symbols found in the Produces' body will also be tested for #$ prefix, and offsets/symtabs extracted.

In addition to Toplevel and Produces support, the Denotes clause was updated to use a reader rather than the Element API, so that it can access the symbol IDs to complete its data model tests.

Additionally

I also used this PR to follow up on #828, and rolled the typed null handling into a read_null function and reset the ion_type back to its original for consistency. I'll follow up with a PR to return the underlying type from ion_type for all value implementations.

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

zslayton · 2024-09-11T18:00:22Z

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

-            return Ok(ValueRef::Null(self.ion_type()));
+            return self.read_null()?.resolve(context);


The resolve method requires a giant match over all of the Ion types (see below). Since we already know it's a (possibly typed) null, I suggest return ValueRef::Null(self.read_null())?;

zslayton · 2024-09-11T18:00:57Z

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

@@ -218,7 +213,7 @@ impl<'top> LazyRawValue<'top, BinaryEncoding_1_1> for &'top LazyRawBinaryValue_1
            }

            if value.is_null() {
-                return Ok(ValueRef::Null(value.ion_type()));
+                return value.read_null()?.resolve(context);


Same thought here RE: ValueRef::Null(...).

zslayton · 2024-09-11T18:01:22Z

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

@@ -354,6 +349,16 @@ impl<'top> LazyRawBinaryValue_1_1<'top> {
        <&'top Self as LazyRawValue<'top, BinaryEncoding_1_1>>::read(&self)
    }

+    fn read_null(&self) -> IonResult<RawValueRef<'top, BinaryEncoding_1_1>> {
+        let tpe = if self.encoded_value.header.type_code() == OpcodeType::TypedNull {


Suggested change

let tpe = if self.encoded_value.header.type_code() == OpcodeType::TypedNull {

let ion_type = if self.encoded_value.header.type_code() == OpcodeType::TypedNull {

?

zslayton · 2024-09-11T18:03:19Z

tests/conformance_dsl/continuation.rs

-                    }
-                }
-            }
+            Continuation::Produces(produces) => produces.evaluate(ctx),


Nice, this was helpful.

zslayton · 2024-09-11T18:04:08Z

tests/conformance_dsl/continuation.rs

@@ -230,6 +209,66 @@ pub(crate) struct EachBranch {
    fragment: Fragment,
 }

+#[derive(Clone, Debug, Default)]


Could add docs and a reference link to these like you have on the other types?

zslayton · 2024-09-11T18:12:51Z

tests/conformance_dsl/mod.rs

@@ -306,3 +306,26 @@ pub(crate) fn parse_text_exp<'a, I: IntoIterator<Item=&'a Element>>(elems: I) ->
    let val_string = bytes.iter().map(|v| unsafe { String::from_utf8_unchecked(v.to_vec()) }).collect();
    Ok(val_string)
 }
+
+/// Parses absent symbol notation from a symbol within a Toplevel fragment, or produces
+/// Continuation. The notation is '#$<id>' for an absent symbol id, or '#$<name>#<id>' for a symbol


I thought #$<id> was the syntax for a symbol ID to be evaluated within the context being created instead of the current one. Is that what you mean by "absent"?

Yea, that's what I mean, that it's absent from the test document's context.

zslayton · 2024-09-11T18:16:13Z

tests/conformance_dsl/fragment.rs

+// newtype to handle writing an Element, after we check to make sure it's not a symbol that has our
+// special absent symbol sauce.
+#[derive(Debug)]
+pub(crate) struct ProxyElement<'a>(pub &'a Element);


Could you add a high-level comment to this type/file that explains the general motivation behind and process for creating/reading/writing ProxyElement instances?

zslayton · 2024-09-11T18:18:01Z

tests/conformance_dsl/model.rs

-                    true
-                }
-            }
+            ModelValue::Struct(_) => panic!("should not be using element eq for struct"),


What is it supposed to be using?

I transitioned over to using a regular reader instead of the element API in order to access symbol IDs, so the handling of structs, lists, and s-expressions moved over to the LazyValue PartialEq. I cleaned this one up a bit and dropped a comment. Next time I touch this I'll probably get rid of the PartialEqs entirely in favor of something that can return a Result, and get rid of this separation.

zslayton · 2024-09-11T18:18:26Z

tests/conformance_dsl/model.rs

+    }
+}
+
+impl<T: ion_rs::Decoder> PartialEq<ion_rs::LazyValue<'_, T>> for &ModelValue {


Could you do a comments pass?

zslayton · 2024-09-11T18:18:59Z

tests/conformance_tests.rs

@@ -11,6 +12,21 @@ use std::str::FromStr;
 mod implementation {
    use super::*;

+    #[test]
+    fn test_absent_symbol() {


Do we also need tests for the #table#$id syntax?

…; Refactor comparisons

nirosys · 2024-09-12T11:48:35Z

As I was reviewing everything I realized I had somehow left out the actual symbol table resolutions. I had stashed some stuff when rolling back some changes and I guess it got lost in the mix. So the latest commit adds in the building of the ion-tests catalog, and validates symbol text between the referenced symbol table, and the current context.

I also tried to document everything mentioned, and added in a couple more tests for 'absent symbols'.

zslayton

Looks good. Please open an issue for TODO items you mentioned in the comments.

zslayton · 2024-09-16T17:25:45Z

tests/conformance_dsl/context.rs

+    symbol_tables: Vec<SharedSymbolTable>, // To build Catalogs when needed, Catalog doesn't
+                                           // require Clone.


~~I'm not sure I understand the comment. Doesn't require Clone for...? Is it something we should change or track?~~

Should this be a MapCatalog instead so you can clone() it?

zslayton · 2024-09-16T17:29:29Z

tests/conformance_dsl/context.rs

+
+    /// Returns the symbol at the provided offset `offset` within the symbol table named `symtab`,
+    /// if either the symbol table, or the offset, is not valid then None is returned.
+    pub fn get_symbol_from_table<S: AsRef<str>>(&self, symtab: S, offset: SymbolId) -> Option<Symbol> {


Suggested change

pub fn get_symbol_from_table<S: AsRef<str>>(&self, symtab: S, offset: SymbolId) -> Option<Symbol> {

pub fn get_symbol_from_table<S: AsRef<str>>(&self, symtab_name: S, offset: SymbolId) -> Option<Symbol> {

zslayton · 2024-09-16T17:30:07Z

tests/conformance_dsl/continuation.rs

+    /// Creates a reader using the provided context, and compares the read values from the input
+    /// document with the elements specified in the associated Produces clause for equality.


Very helpful, thanks.

zslayton · 2024-09-16T17:34:15Z

tests/conformance_dsl/fragment.rs

+//! read by the ion reader they're treated as normal quoted symbols. When the Toplevel fragment is
+//! encoded however, each Element read from the Toplevel clause is wrapped within a `ProxyElement`,
+//! which by implementing `WriteAsIon` is able to recursively look at all values being written to
+//! the test's input and capture any symbols that match the unknown symbol notation.


A (perhaps simpler?) way to achieve this would have been to make a Reader wrapper that provided its own implementation of the ElementReader trait.

I'll look into this more and sync up with you when I start working on #839, maybe I can sneak changes in there if it simplifies things.

Update conformance testing DSL to support unknown symbols

978fbd3

nirosys marked this pull request as ready for review September 9, 2024 10:42

zslayton reviewed Sep 11, 2024

View reviewed changes

Address PR feedback; Add in missing catalog build & symtab resolution…

9eab933

…; Refactor comparisons

zslayton approved these changes Sep 16, 2024

View reviewed changes

nirosys mentioned this pull request Sep 30, 2024

Refactor conformance dsl to eliminate PartialEq use. #839

Open

nirosys added 2 commits September 30, 2024 12:08

Merge branch 'main' into rgiliam/conformance_absent_symbols

32fbab4

Merge branch 'main' into rgiliam/conformance_absent_symbols

0c2f80d

nirosys merged commit ec13464 into amazon-ion:main Oct 16, 2024
26 of 27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update conformance testing DSL to support unknown symbols #833

Update conformance testing DSL to support unknown symbols #833

nirosys commented Sep 9, 2024 •

edited

Loading

zslayton Sep 11, 2024

zslayton Sep 11, 2024

zslayton Sep 11, 2024

zslayton Sep 11, 2024

zslayton Sep 11, 2024

zslayton Sep 11, 2024

nirosys Sep 12, 2024

zslayton Sep 11, 2024

zslayton Sep 11, 2024

nirosys Sep 12, 2024 •

edited

Loading

zslayton Sep 11, 2024

zslayton Sep 11, 2024

nirosys commented Sep 12, 2024

zslayton left a comment

zslayton Sep 16, 2024

zslayton Sep 16, 2024

zslayton Sep 16, 2024

zslayton Sep 16, 2024

nirosys Sep 30, 2024

		return Ok(ValueRef::Null(self.ion_type()));
		return self.read_null()?.resolve(context);

	let tpe = if self.encoded_value.header.type_code() == OpcodeType::TypedNull {
	let ion_type = if self.encoded_value.header.type_code() == OpcodeType::TypedNull {

		symbol_tables: Vec<SharedSymbolTable>, // To build Catalogs when needed, Catalog doesn't
		// require Clone.

	pub fn get_symbol_from_table<S: AsRef<str>>(&self, symtab: S, offset: SymbolId) -> Option<Symbol> {
	pub fn get_symbol_from_table<S: AsRef<str>>(&self, symtab_name: S, offset: SymbolId) -> Option<Symbol> {

		/// Creates a reader using the provided context, and compares the read values from the input
		/// document with the elements specified in the associated Produces clause for equality.

Update conformance testing DSL to support unknown symbols #833

Update conformance testing DSL to support unknown symbols #833

Conversation

nirosys commented Sep 9, 2024 • edited Loading

Details

Additionally

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

nirosys Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirosys commented Sep 12, 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

nirosys commented Sep 9, 2024 •

edited

Loading

nirosys Sep 12, 2024 •

edited

Loading