Removes generating a separate class/struct for nested sequence and scalar type #111

desaikd · 2024-05-30T15:56:30Z

Issue #110:

Description of changes:

This PR works on generating nested sequence and scalar types in the same class/struct.

What is the change?

Previously, code generator would generate nested sequence or scalar types as a separate nested class or struct in a nested module. But this new change does not create a new class rather maps these nested sequence or scalar type in the same class/struct that uses it.

e.g. For a schema as below:

type::{
   name: struct_type,
   fields:  {
      foo: { type: list, element: int }, // nested sequence type
      bar: { type: string } // nested scalar type
   }
}

Previously generated code:

class StructType {
  private NestedType1 foo;
  private NestedType2 bar; 
   ...

   class NestedType1 {
      private ArrayList<Integer> value;
      ...
   }

   class NestedType1 {
      private String value;
      ...
   }
}

Code generated with this PR changes:

class StructType {
  private ArrayList<Integer> foo;
  private String bar; 
   ...
}

(Note: For example purpose I have only used java code but same is the case with Rust. Also, top level sequence/scalar top would still generate a separate class/struct. For more information on complete newly generated code see here)

Note for reviewer:

This PR also adds a new data model that can be used by code generator to render templates. The current approach can not generate nested sequence types like {type: list, element: {type: sexp, element: int} }. This will be resolved with the new data model as well as the new data model makes it easier to store all the information used by templates in a single place. This Pr just introduces the model but doesn't use it in implementation yet. The change for usage will be added in a separate PR.

Generated code:

Generated code for Java can be found here.
Generated code for Rust can be found here.
(You can find corresponding ISL files and Ion files in code-gen-projects directory)

List of changes:

Removes generating a separate class for nested sequence/scalar type
- Adds changes to Field to store AbstractDataType information of the
  field value
- Modified to map_constraint_to_abstract_data_type to consider nested
  sequence/scalar type and change AsbtractDataType and tera_fields
  accordingly
- Modified template to call util_macros for reading or writing a field
  as sequence
Removes generating separate struct for nested sequence/scalar types
- Modified Rust templates to consider util_macros when reading or
  writing a nested sequence type
Adds new tests for sequence and scalar types
Adds separate template files for sequence, scalar and struct in Java and Rust
Adds new data model for code generator to render templates

Test:

Added new test schema and input files for this change.

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

* Adds changes to `Field` to store `AbstractDataType` information of the field value * Modified to `map_constraint_to_abstract_data_type` to consider nested sequence/scalar type and change `AsbtractDataType` and `tera_fields` accordingly * Modified template to call `util_macros` for reading or writing a field as sequence

* Modified Rust templates to consider `util_macros` when reading or writing a nested sequence type

…and Rust

desaikd · 2024-05-30T20:18:26Z

src/bin/ion/commands/beta/generate/templates/java/class.templ

@@ -56,70 +58,36 @@ public class {{ target_kind_name }} {
                null
            {% endif %};
        {% endfor %}
-        {% if abstract_data_type == "Value"%}


(🗺️ PR tour) This template used to contain all abstract data types templates but now it separated into 3 templates.

class.templ (For struct type)

scalar.templ

sequence.templ

desaikd · 2024-05-30T20:18:34Z

src/bin/ion/commands/beta/generate/templates/java/class.templ

+                                                    {% if field.value_type is containing("ArrayList") %}
+                                                        {{ util_macros::read_as_sequence(field=field) }}


(🗺️ PR tour) This statement is modified to read fields that are of type ArrayList using util_macros write_as_sequence. Previously it was not possible for a field in struct to be of sequence type(e.g. It would be inside some nested type NestedtypeX). But with this change it is possible to have fields with sequence type (e.g. ArrayList<Integer>).

desaikd · 2024-05-30T20:18:38Z

src/bin/ion/commands/beta/generate/templates/java/class.templ

+             {% if field.value_type | is_built_in_type == false %}
+                 {% if field.value_type is containing("ArrayList") %}
+                    {{ util_macros::write_as_sequence(field=field) }}


(🗺️ PR tour) This statement is modified to write fields that are of type ArrayList using util_macros write_as_sequence. Previously it was not possible for a field in struct to be of sequence type(e.g. It would be inside some nested type NestedtypeX). But with this change it is possible to have fields with sequence type (e.g. ArrayList<Inetger>).

desaikd · 2024-05-30T20:18:42Z

src/bin/ion/commands/beta/generate/templates/java/scalar.templ

@@ -0,0 +1,69 @@
+package {{ namespace }};


(🗺️ PR tour) Contents of files java/scalar.templ, java/sequence.templ, rust/scalar.templ, rust/sequence.templ are copied from java/class.templ and rust/struct.templ respectively.

desaikd · 2024-05-30T20:18:51Z

src/bin/ion/commands/beta/generate/templates/rust/struct.templ

@@ -29,106 +31,52 @@ pub mod  {{ target_kind_name | snake }} {

        pub fn read_from(reader: &mut Reader) -> SerdeResult<Self> {
            let mut abstract_data_type = {{ target_kind_name }}::default();
-            {% if abstract_data_type == "Value"%}


(🗺️ PR tour) This template used to contain all abstract data types templates but now it separated into 3 templates.

struct.templ (For struct type)

scalar.templ

sequence.templ

desaikd · 2024-05-30T20:18:55Z

src/bin/ion/commands/beta/generate/templates/rust/struct.templ

+                            {% if field.value_type | is_built_in_type == false %}
+                                {% if field.value_type is containing("Vec") %}
+                                     "{{ field.name }}" => { {{ util_macros::read_as_sequence(field=field) }} }


🗺️ PR tour) This statement is modified to read fields that are of type Vec using util_macros write_as_sequence. Previously it was not possible for a field in struct to be of sequence type(e.g. It would be inside some nested type NestedtypeX). But with this change it is possible to have fields with sequence type (e.g. Vec<i64>).

desaikd · 2024-05-30T20:18:58Z

src/bin/ion/commands/beta/generate/templates/rust/struct.templ

+                {% if field.value_type | is_built_in_type == false %}
+                    {% if field.value_type is containing("Vec") %}
+                        {{ util_macros::write_as_sequence(field=field) }}


(🗺️ PR tour) This statement is modified to write fields that are of type Vec using util_macros write_as_sequence. Previously it was not possible for a field in struct to be of sequence type(e.g. It would be inside some nested type NestedtypeX). But with this change it is possible to have fields with sequence type (e.g. Vec<i64>).

desaikd · 2024-05-30T20:19:04Z

src/bin/ion/commands/beta/generate/utils.rs

+    Struct,   // Represents a template for a Rust struct or Java class with Ion struct value
+    Sequence, // Represents a template for a Rust struct or Java class with Ion sequence value
+    Scalar,   // Represents a template for a Rust struct or Java class with Ion scalar value


(🗺️ PR tour) Add new templates names here to use template based on abstract data type.

desaikd · 2024-05-30T20:19:07Z

tests/cli.rs

@@ -279,8 +279,8 @@ fn test_write_all_values(#[case] number: i32, #[case] expected_output: &str) ->
         }
        }
    "#,
-    &["nested_type: NestedType1"],
-    &["pub fn nested_type(&self) -> &NestedType1 {"]
+    &["nested_type: i64"],


(🗺️ PR tour) This file contains changes for having nested sequence type/scalar type in the same struct/class without generating separate structure.

code-gen-projects/java/code-gen-demo/src/test/java/org/example/CodeGenTest.java

popematt · 2024-05-30T22:47:42Z

src/bin/ion/commands/beta/generate/generator.rs

+                    let mut isl_type_name = value.type_reference().name();
+
+                    if let Some(type_reference_name) = &type_name {
+                        if type_reference_name.contains("NestedType") {


If this is a short-term hack, it's okay. It's a problem if this is here long term because there's nothing stopping a user from creating a type named NestedType.

Yes, with the model change(Likely next PR) I think we can add a field in the model suggesting whether this is an inline type or not. And that should help with these kind of checks.

popematt · 2024-05-30T23:19:10Z

src/bin/ion/commands/beta/generate/model.rs

+/// A target-language-agnostic data type that determines which template(s) to use for code generation.
+#[allow(dead_code)]
+#[derive(Debug, Clone, PartialEq, Serialize)]
+pub enum CodeGenType {


Suggestion—if you create types for each of the variants, then you can require specific types in the signatures of various functions.

pub enum CodeGenType { Value(Value), Sequence(Sequence), // etc. }

Good idea! Will change that.

popematt · 2024-05-30T23:19:29Z

src/bin/ion/commands/beta/generate/model.rs

+    //    value: i64
+    // }
+    // ```
+    Value {


suggestion—Scalar?

popematt · 2024-05-30T23:20:53Z

src/bin/ion/commands/beta/generate/model.rs

+    // }
+    // ```
+    Value {
+        isl_type_name: String,


If this is actually the name from ISL, then it should probably be Option<String>. If this is guaranteed to always have a name, then perhaps it should just be type_name.

Good catch! I will update this to be Option<String> instead.

popematt · 2024-05-30T23:39:14Z

src/bin/ion/commands/beta/generate/templates/java/nested_type.templ

@@ -74,7 +76,11 @@
                                                            reader.{{ field.value_type | camel }}Value();
                                                       {% endif %}
                                                    {% else %}
-                                                        {{ field.value_type }}.readFrom(reader);
+                                                        {% if field.value_type is containing("ArrayList") %}


Will this break if someone creates an ISL type like MyArrayListener?

It will. I will be changing the data model defined as per the current PR in model.rs. That has a reference to CodeGenType and based on this enum variant these checks can be defined properly.

src/bin/ion/commands/beta/generate/templates/java/util_macros.templ

popematt · 2024-06-03T19:03:52Z

src/bin/ion/commands/beta/generate/templates/rust/nested_type.templ

                    {% endif %}
                {% endfor %}
            {% elif abstract_data_type is object and abstract_data_type is containing("Structure") %}
                writer.step_in(IonType::Struct)?;
                {% for field in fields %}
                writer.set_field_name("{{ field.name }}");
                    {% if field.value_type | is_built_in_type == false %}
-                        self.{{ field.name | snake }}.write_to(writer)?;
+                        {% if field.value_type is containing("Vec") %}


If I define an ISL type such as FeatureVector or FlowVector, this is probably going to do surprising things.

You are right. which is why with the new model we can just check the data model type.

desaikd · 2024-06-03T19:44:15Z

As per offline discussion, I am removing model.rs from this PR and will be added as a separate PR.

desaikd added 5 commits May 29, 2024 12:32

Removes generating separate struct for nested sequence/scalar types

706e04d

* Modified Rust templates to consider `util_macros` when reading or writing a nested sequence type

Adds new tests for sequence and scalar types

c4b327f

Adds separate template files for sequence, scalar and struct in Java …

6160cc6

…and Rust

Adds new data model for code generator to render templates

bddc3a1

desaikd marked this pull request as ready for review May 30, 2024 16:09

desaikd requested a review from popematt May 30, 2024 16:12

desaikd commented May 30, 2024

View reviewed changes

popematt reviewed Jun 3, 2024

View reviewed changes

desaikd added 3 commits June 3, 2024 12:52

Removes new model for code generator

d1d8504

Adds suggested changes

f11f1ab

Merge branch master

1802ff6

desaikd force-pushed the code-gen-nested-types branch from 8e98264 to 1802ff6 Compare June 3, 2024 23:27

popematt approved these changes Jun 5, 2024

View reviewed changes

desaikd merged commit 4959fdf into amazon-ion:master Jun 10, 2024
4 checks passed

desaikd mentioned this pull request Jun 10, 2024

Adds changes for updating template resources #122

Merged

desaikd mentioned this pull request Jun 25, 2024

Sequence and scalar nested types should not generate a separate class/struct #110

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes generating a separate class/struct for nested sequence and scalar type #111

Removes generating a separate class/struct for nested sequence and scalar type #111

desaikd commented May 30, 2024 •

edited

Loading

desaikd May 30, 2024

desaikd May 30, 2024

desaikd May 30, 2024

desaikd May 30, 2024

desaikd May 30, 2024

desaikd May 30, 2024

desaikd May 30, 2024 •

edited

Loading

desaikd May 30, 2024

desaikd May 30, 2024

popematt May 30, 2024

desaikd Jun 3, 2024

popematt May 30, 2024

desaikd Jun 3, 2024

popematt May 30, 2024

popematt May 30, 2024

desaikd Jun 3, 2024

popematt May 30, 2024

desaikd Jun 3, 2024

popematt Jun 3, 2024

desaikd Jun 3, 2024

desaikd commented Jun 3, 2024

		{% if field.value_type is containing("ArrayList") %}
		{{ util_macros::read_as_sequence(field=field) }}

Removes generating a separate class/struct for nested sequence and scalar type #111

Removes generating a separate class/struct for nested sequence and scalar type #111

Conversation

desaikd commented May 30, 2024 • edited Loading

Issue #110:

Description of changes:

What is the change?

Note for reviewer:

Generated code:

List of changes:

Test:

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

desaikd May 30, 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

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

desaikd commented Jun 3, 2024

desaikd commented May 30, 2024 •

edited

Loading

desaikd May 30, 2024 •

edited

Loading