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

Fix recursive viewing of parent class in explorer tree of query builder #1180

Merged
merged 1 commit into from Aug 3, 2022

Conversation

gayathrir11
Copy link
Contributor

@gayathrir11 gayathrir11 commented May 24, 2022

Summary

Closes #1172

How did you test this change?

  • Test(s) added
  • Manual testing (please provide screenshots/recordings)
  • No testing (please provide an explanation)

Model Data for Association

###Relational
Database my::db
(
  Table PersonTable
  (
    NAME CHAR(200),
    FIRMID INTEGER
  )
  Table FirmTable
  (
    FIRMNAME CHAR(200),
    ID INTEGER
  )

  Join Firm_Person(PersonTable.FIRMID = FirmTable.ID)
)


###Pure
Class my::Firm
{
  legalName: String[1];
  id: Integer[1];
}

Class my::Person
{
  name: String[1];
  firmID: Integer[1];
}

Association my::Employment
{
  firm: my::Firm[*];
  employees: my::Person[*];
}


###Mapping
Mapping my::map
(
  my::Person[per1]: Relational
  {
    ~mainTable [my::db]PersonTable
    name: [my::db]PersonTable.NAME
  }
  my::Firm[fir1]: Relational
  {
    ~mainTable [my::db]FirmTable
    legalName: [my::db]FirmTable.FIRMNAME
  }

  my::Employment: Relational
  {
    AssociationMapping
    (
      firm[per1,fir1]: [my::db]@Firm_Person,
      employees[fir1,per1]: [my::db]@Firm_Person
    )
  }
)

pic

@changeset-bot
Copy link

changeset-bot bot commented May 24, 2022

🦋 Changeset detected

Latest commit: d841544

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages
Name Type
@finos/legend-application-query Patch
@finos/legend-application-query-bootstrap Patch
@finos/legend-extension-application-studio-query-builder Patch
@finos/legend-extension-dsl-data-space Patch
@finos/legend-application-query-deployment Patch
@finos/legend-application-studio-bootstrap Patch
@finos/legend-application-taxonomy-bootstrap Patch
@finos/legend-application-taxonomy Patch
@finos/legend-manual-tests Patch
@finos/legend-application-studio-deployment Patch
@finos/legend-application-taxonomy-deployment Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@finos-cla-bot finos-cla-bot bot added the cla-present CLA Signed label May 24, 2022
@gayathrir11 gayathrir11 force-pushed the recursive_bug branch 2 times, most recently from f398d41 to c9c89b8 Compare May 25, 2022 09:12
@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #1180 (d841544) into master (64e587e) will increase coverage by 0.16%.
The diff coverage is 68.75%.

@@            Coverage Diff             @@
##           master    #1180      +/-   ##
==========================================
+ Coverage   41.73%   41.90%   +0.16%     
==========================================
  Files        1187     1176      -11     
  Lines       52595    52444     -151     
  Branches    11954    11928      -26     
==========================================
+ Hits        21952    21976      +24     
+ Misses      30573    30397     -176     
- Partials       70       71       +1     
Impacted Files Coverage Δ
...src/components/QueryBuilderPropertySearchPanel.tsx 46.83% <0.00%> (-0.30%) ⬇️
...query/src/components/QueryBuilderExplorerPanel.tsx 68.23% <50.00%> (+0.11%) ⬆️
...tion-query/src/stores/QueryBuilderExplorerState.ts 70.64% <75.00%> (-0.19%) ⬇️
...src/stores/QueryBuilderPropertySearchPanelState.ts 51.65% <100.00%> (ø)
...pure/DSLPersistence_PureProtocolProcessorPlugin.ts 78.43% <0.00%> (-21.57%) ⬇️
...ableElements/persistence/V1_DSLPersistence_Sink.ts 50.00% <0.00%> (-21.43%) ⬇️
...ageableElements/persistence/DSLPersistence_Sink.ts 66.66% <0.00%> (-8.34%) ⬇️
...n-query/src/components/QueryBuilderFilterPanel.tsx 56.42% <0.00%> (-0.66%) ⬇️
...ure/MappingGeneration_PureGraphManagerExtension.ts
.../V1_MappingGeneration_PureGraphManagerExtension.ts
... and 13 more

@MauricioUyaguari
Copy link
Member

Reviewing some of the work yannan is doing and this might actually be taken care of with the mapping algo we will be using now.
@YannanGao-gs could you review this PR to confirm whether we would still need this if we switch to using the mapping algorithm you are implementing via PURE? Looking at the mappings for both grammars, i think the mapping algo would take care of this.

@YannanGao-gs
Copy link
Contributor

Reviewing some of the work yannan is doing and this might actually be taken care of with the mapping algo we will be using now. @YannanGao-gs could you review this PR to confirm whether we would still need this if we switch to using the mapping algorithm you are implementing via PURE? Looking at the mappings for both grammars, i think the mapping algo would take care of this.

The mapping algorithm won't avoid loop. We have to do it manually if this is needed.

@YannanGao-gs
Copy link
Contributor

YannanGao-gs commented Jun 1, 2022

Hello @gayathrir11,
I think your pr works well on the model above. But it will fail on the below one where there is a parent class (inheritance)in the loop.

###Relational
Database model::Test
(
  Table FirmTable
  (
    id INTEGER PRIMARY KEY,
    Legal_name VARCHAR(200)
  )
  Table PersonTable
  (
    id INTEGER PRIMARY KEY,
    firm_id INTEGER,
    firstName VARCHAR(200),
    lastName VARCHAR(200)
  )
  Table AddressTable
  (
    id INTEGER PRIMARY KEY,
    firm_id INTEGER,
    name VARCHAR(200)
  )

  Join FirmPerson(PersonTable.firm_id = FirmTable.id)
)


###Pure
Class model::LegalEntity
{
  employees: model::Person[1..*];
}

Class model::Firm extends model::LegalEntity
{
  legalName: String[1];
}

Class model::Person
{
  firstName: String[1];
  lastName: String[1];
  firms: model::Firm[1..*];
}


###Mapping
Mapping model::NewMapping
(
  *model::Firm: Relational
  {
    ~primaryKey
    (
      [model::Test]FirmTable.id
    )
    ~mainTable [model::Test]FirmTable
    employees[model_Person]: [model::Test]@FirmPerson
  }
  *model::Person: Relational
  {
    ~primaryKey
    (
      [model::Test]PersonTable.id
    )
    ~mainTable [model::Test]PersonTable
    firstName: [model::Test]PersonTable.firstName,
    lastName: [model::Test]PersonTable.lastName,
    firms[model_Firm]: [model::Test]@FirmPerson
  }
  *model::LegalEntity: Operation
  {
    meta::pure::router::operations::inheritance_OperationSetImplementation_1__SetImplementation_MANY_()
  }
)

@MauricioUyaguari
Copy link
Member

Hello @gayathrir11, I think your pr works well on the model above. But it will fail on the below one where there is a parent class (inheritance)in the loop.

###Relational
Database model::Test
(
  Table FirmTable
  (
    id INTEGER PRIMARY KEY,
    Legal_name VARCHAR(200)
  )
  Table PersonTable
  (
    id INTEGER PRIMARY KEY,
    firm_id INTEGER,
    firstName VARCHAR(200),
    lastName VARCHAR(200)
  )
  Table AddressTable
  (
    id INTEGER PRIMARY KEY,
    firm_id INTEGER,
    name VARCHAR(200)
  )

  Join FirmPerson(PersonTable.firm_id = FirmTable.id)
)


###Pure
Class model::LegalEntity
{
  employees: model::Person[1..*];
}

Class model::Firm extends model::LegalEntity
{
  legalName: String[1];
}

Class model::Person
{
  firstName: String[1];
  lastName: String[1];
  firms: model::Firm[1..*];
}


###Mapping
Mapping model::NewMapping
(
  *model::Firm: Relational
  {
    ~primaryKey
    (
      [model::Test]FirmTable.id
    )
    ~mainTable [model::Test]FirmTable
    employees[model_Person]: [model::Test]@FirmPerson
  }
  *model::Person: Relational
  {
    ~primaryKey
    (
      [model::Test]PersonTable.id
    )
    ~mainTable [model::Test]PersonTable
    firstName: [model::Test]PersonTable.firstName,
    lastName: [model::Test]PersonTable.lastName,
    firms[model_Firm]: [model::Test]@FirmPerson
  }
  *model::LegalEntity: Operation
  {
    meta::pure::router::operations::inheritance_OperationSetImplementation_1__SetImplementation_MANY_()
  }
)

@YannanGao-gs for your example, could you provide what the mapping algo provides for that example so @gayathrir11 and I could better understand what is and is not taken of by the mapping algo. Thanks.

@YannanGao-gs
Copy link
Contributor

MappingAnalysisResult

  • model data with association
{
    "mappedEntities": [
        {
            "name": "my::Person",
            "properties": [
                {
                    "name": "name"
                },
                {
                    "name": "firm",
                    "entityName": "my::Firm"
                }
            ],
            "nameToProperty": {}
        },
        {
            "name": "my::Firm",
            "properties": [
                {
                    "name": "employees",
                    "entityName": "my::Person"
                },
                {
                    "name": "legalName"
                }
            ],
            "nameToProperty": {}
        }
    ],
    "nameToEntity": {}
}
  • model data for non-assocation
{
    "mappedEntities": [
        {
            "name": "my::Person",
            "properties": [
                {
                    "name": "id"
                },
                {
                    "name": "name"
                },
                {
                    "name": "firm",
                    "entityName": "my::Firm"
                }
            ],
            "nameToProperty": {}
        },
        {
            "name": "my::Firm",
            "properties": [
                {
                    "name": "name"
                },
                {
                    "name": "person",
                    "entityName": "my::Person"
                },
                {
                    "name": "firmId"
                }
            ],
            "nameToProperty": {}
        }
    ],
    "nameToEntity": {}
}
  • inheritance model above
{
    "mappedEntities": [
        {
            "name": "model::Firm",
            "properties": [
                {
                    "name": "employees",
                    "entityName": "model::Person"
                }
            ],
            "nameToProperty": {}
        },
        {
            "name": "model::LegalEntity",
            "properties": [
                {
                    "name": "employees",
                    "entityName": "model::Person"
                },
                {
                    "name": "firm",
                    "entityName": "@model::Firm",
                    "subType": "model::Firm"
                }
            ],
            "nameToProperty": {}
        },
        {
            "name": "@model::Firm",
            "properties": [
                {
                    "name": "employees",
                    "entityName": "model::Person"
                }
            ],
            "nameToProperty": {}
        },
        {
            "name": "model::Person",
            "properties": [
                {
                    "name": "firms",
                    "entityName": "model::Firm"
                },
                {
                    "name": "lastName"
                },
                {
                    "name": "firstName"
                }
            ],
            "nameToProperty": {}
        }
    ],
    "nameToEntity": {}
}

@akphi
Copy link
Contributor

akphi commented Jun 15, 2022

@gayathrir11 Let's wait for @YannanGao-gs #1186 to figure out better specs for this

@akphi
Copy link
Contributor

akphi commented Jul 7, 2022

@gayathrir11 I think we can address this now as #1186 is merged

@MauricioUyaguari
Copy link
Member

@gayathrir11 I think we can address this now as #1186 is merged

@gayathrir11 @YannanGao-gs yes, let's please ensure this is taken care of now that mapping analytics has been merged.

@gayathrir11 gayathrir11 force-pushed the recursive_bug branch 2 times, most recently from 0f33e5c to 8dede60 Compare July 14, 2022 09:55
@akphi
Copy link
Contributor

akphi commented Jul 17, 2022

@gayathrir11 there's a breaking test

@gayathrir11 gayathrir11 force-pushed the recursive_bug branch 2 times, most recently from 68a7bc5 to 62971ed Compare July 20, 2022 07:58
@akphi
Copy link
Contributor

akphi commented Jul 20, 2022

Look at this example
###Relational
Database my::db
(
  Table PersonTable
  (
    NAME CHAR(200),
    FIRMID INTEGER
  )
  Table FirmTable
  (
    FIRMNAME CHAR(200),
    ID INTEGER
  )

  Join Firm_Person(PersonTable.FIRMID = FirmTable.ID)
)


###Pure
Class my::Firm
{
  legalName: String[1];
  id: Integer[1];
  employeees2: my::Person[1..*];
}

Class my::Person
{
  name: String[1];
  firmID: Integer[1];
}

Association my::Employment
{
  firm: my::Firm[*];
  employees: my::Person[*];
}


###Mapping
Mapping my::map
(
  my::Person[per1]: Relational
  {
    ~mainTable [my::db]PersonTable
    name: [my::db]PersonTable.NAME
  }
  my::Firm[fir1]: Relational
  {
    ~mainTable [my::db]FirmTable
    legalName: [my::db]FirmTable.FIRMNAME,
    employeees2[per1]: [my::db]@Firm_Person
  }

  my::Employment: Relational
  {
    AssociationMapping
    (
      firm[per1,fir1]: [my::db]@Firm_Person,
      employees[fir1,per1]: [my::db]@Firm_Person
    )
  }
)

I felt like this ticket is really about handling association, while the case of employees2 chain should be properly cut-off by reading the mapping data. Anyway, @gayathrir11 and @YannanGao-gs please work together to define the specs for this more clearly and execute. Thanks!

@YannanGao-gs
Copy link
Contributor

YannanGao-gs commented Jul 25, 2022

@gayathrir11 Looks good to me

@gayathrir11 gayathrir11 force-pushed the recursive_bug branch 3 times, most recently from fdf216b to 7399de1 Compare August 3, 2022 09:21
@akphi akphi merged commit c2c308a into finos:master Aug 3, 2022
@gayathrir11 gayathrir11 deleted the recursive_bug branch March 3, 2023 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-present CLA Signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable recursive viewing of parent class in explorer tree of query builder for better user experience
4 participants