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

Adds changes for sequence data type #144

Merged
merged 3 commits into from
Sep 25, 2024

Conversation

desaikd
Copy link
Contributor

@desaikd desaikd commented Sep 18, 2024

Issue #, if available:

Description of changes:

This PR works on adding new model changes following up on #135. This Pr only contains changes for Sequence. This PR disables Rust support until the entire functionality for the new model change is supported and uses feature branch new-model-changes as target.

List of changes:

  • Generator changes:

    • Adds build_wrapped_sequence_from_constraints which is used for constructing named sequence data type as data model.
    • Adds build_sequence_from_constraints which is used for constructing nested sequence data type as data model. (Doesn't have a name)
    • Adds is_nested flag in convert_isl_type_def_to_data_model_node which can be used to either call wrapped sequence or sequence ADT construction.
    • Modified sequence construction to move the common logic at the beginning of the method.
  • Model changes:

    • Changes Sequence and WrappedSequence ADT to represent the underlying sequence type with base_type and its name with name field.
    • Adds doc comment changes for the same
  • Adds templates changes for java/sequence.templ

Generated code:

The generated code still stays the same only the template changes to use the new model. (Only added sequence files for checking this change, other files remain same)

Generated Java code can be found here.

Tests:

  • Adds tests for ISL to model conversion.
  • Modifies roundtrip code gen tests for sequence test cases.

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

src/bin/ion/commands/generate/generator.rs Show resolved Hide resolved
src/bin/ion/commands/generate/generator.rs Show resolved Hide resolved
src/bin/ion/commands/generate/generator.rs Show resolved Hide resolved
src/bin/ion/commands/generate/generator.rs Show resolved Hide resolved
src/bin/ion/commands/generate/model.rs Outdated Show resolved Hide resolved
Sequence(Sequence),
// Represents a sequence type which also has anme attached to it and is nominally distinct from its element type.
WrappedSequence(WrappedSequence),
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we going to have to create corresponding Wrapped* variants for all of the non-wrapped variants? Why/why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I think its in general there are only 3 types structure, sequence and scalar. Where only nested structure would have a name but scalar and sequence would not have name. Hence to separate out whether they are named or not I have them as a separate variant. I think having them separate allows us to perform specific operations for these types which will be stored as property instead of entirely different class in the generated code.

e.g. For below ISL:

type::{
   name: my_type,
   fields: {
        foo: {type: list, element: string} // this will not create a new nested class in the generated code
   }
}

Here's the generated code in Java:

class MyType {
   private java.util.ArrayList<String> foo;
   ...
}

whereas for structure type like following:

type::{
   name: my_type,
   fields: {
        foo: {
           fields: {
               bar: string
           }
        } 
   }
}

Here's the generated code in Java, which creates a new nested type for this nested structure:

class MyType {
   private NestedType1 foo;
   ...
   
   class NestedType1 {
       private String bar;
       ...
   }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need the non-wrapped variant of sequence? We have parameterized, fully-qualified type references, so we should be able to model the non-wrapped lists as e.g. java.util.List<org.example.MyGeneratedClass>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WrpapedSequence use FullyQualifiedTypeName (not FullyqualifiedTypeReference). And WrappedSequence have a name, for example:

type::{
   name: foo, // this is the name of the wrapped sequence
   type: list,
   element: string
}

Above wrapped sequence's name will be stored as org.example.Foo .
Whereas Sequence would not have a name, for example:

{type: list, element: string} // does not have a name

We can surely store it as fully-qualified type references and thats what we already we store the element property as FullyWualfiiedTypeReference and then when storing this type into TypeStore we use java.util.ArrayList<String> but it still wouldn't have name (like the wrapped sequence has name as foo)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but the non-wrapped version (Sequence) does not have a name and does not have a class generated for it. Therefore, why do we need to have the unnamed Sequence model? You don't need a reference to the Sequence when you can just use a parameterized reference such as java.util.ArrayList<java.lang.String>.

For example, if you have this schema, the { type: list, element: string } does not need a Sequence to be created to represent it. Instead, the field bar should have the data model equivalent of a reference to java.util.ArrayList<java.lang.String>.

type::{
  name: foo,
  fields: {
    bar: { type: list, element: string }
  }
}

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 have created an issue for this: #145. Need to look at if removing the non-wrapped variants has any requirements for how they are rendered in templates.

Copy link
Contributor

@zslayton zslayton left a comment

Choose a reason for hiding this comment

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

Approved pending changes to the issues Matt raised.

* Sequences should store the fully qualified type reference with
appropriate sequence data type. e.g. For Java, it should store the
element type in an `ArrayList` in the `FullyqualifiedtypeReference`.
* Adds checks for duplicate `element` and `type` constraint
* Adds doc comment changes
Copy link
Contributor

@popematt popematt left a comment

Choose a reason for hiding this comment

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

Please create an issue to follow up on the wrapped vs non wrapped sequences.

@desaikd desaikd merged commit 3154261 into amazon-ion:new-model-changes Sep 25, 2024
3 checks passed
desaikd added a commit to desaikd/ion-cli that referenced this pull request Nov 1, 2024
desaikd added a commit that referenced this pull request Nov 1, 2024
* Adds new model changes for `Structure` (#138)
* Adds new model changes for scalar (#143)
* Adds changes for sequence data type (#144)
* Adds tests for new model changes (#146)
* Adds new model changes for Rust code generation (#147)
* Adds changes for optional/required fields in Java code generation (#148)
* Modifies generated setter API to take nested types instead of nested properties (#153)
* Adds support for builder API generation in Java (#154)
* Adds support for Java enum generation (#158)
* Adds namespace fix for nested sequence and adds support for nested types in Sequence and Scalar ADT (#163)
* Adds changes for nested type naming (#166)
* Adds support for imports in code generation (#169)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants