-
Notifications
You must be signed in to change notification settings - Fork 237
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
support mapping relations to classes #3306
base: master
Are you sure you want to change the base?
Conversation
...core-pure/src/main/resources/core_relational/relational/helperFunctions/helperFunctions.pure
Outdated
Show resolved
Hide resolved
tacn: TableAliasColumnName[1]| let new = $m->get($tacn.alias.name); | ||
assertNotEmpty($new); | ||
let alias = $tacn.alias; | ||
^$tacn(alias = ^$alias(name=$new->toOne()));, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is purely for a bug fix (see fixed PCT tests). I don't use TableAliasColumnName anywhere. Encountered this bug while running some queries during development.
...ompiled-core/src/main/resources/core/pure/protocol/v1_33_0/transfers/valueSpecification.pure
Outdated
Show resolved
Hide resolved
); | ||
} | ||
|
||
function meta::pure::router::store::routing::byPassClusteringInfo(value:Any[1]):Any[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, still need to bypass clustering info before passing to pure2sql flow - https://github.com/gs-rpant1729/legend-engine/blob/4bf3cf76254dd9ec64a1c790512d6bb7b6262d82/legend-engine-xts-relationalStore/legend-engine-xt-relationalStore-generation/legend-engine-xt-relationalStore-pure/legend-engine-xt-relationalStore-core-pure/src/main/resources/core_relational/relational/pureToSQLQuery/pureToSQLQuery.pure#L3741
@@ -34,7 +37,17 @@ import meta::pure::store::*; | |||
// ========================================================================================= | |||
function meta::pure::router::store::routing::getRoutingStrategyFromMappingAndRuntime(mapping:Mapping[1], runtime:Runtime[1]):StoreMappingRoutingStrategy[1] | |||
{ | |||
getRoutingStrategyFromMappingAndRuntime($mapping, $runtime, []); | |||
^StoreMappingRoutingStrategy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this changing functionality? Previously it would have gone to function below to help with model joins based on runtime locality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the function was unused before, I repurposed it to return just the strategy object. Both usages of this function are in the same file, the overloaded version of the function (with extensions param) is the one used externally. Have made this function private to avoid incorrect usage.
@@ -7621,7 +7651,7 @@ function meta::relational::functions::pureToSqlQuery::addExtraJoinColumns(tableA | |||
let joinTableAliasColumns = $j.operation->extractTableAliasColumns()->filter(c|$c.alias==$tableAlias)->removeDuplicates(); | |||
let updatedRelationalElement = $tableAlias.relationalElement->match([s:SelectSQLQuery[1] | let existingCols = $s.columns->map(col| $col->match([tac:TableAliasColumn[1] | $tac.column.name, | |||
a:Alias[1] | $a.name | |||
])); | |||
])->stripMatchingQuotes()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should discuss on quotes and TableAliasColumnName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to strip quotes because we add them here for some reason - https://github.com/finos/legend-engine/blob/master/legend-engine-xts-relationalStore/legend-engine-xt-relationalStore-generation/legend-engine-xt-relationalStore-pure/legend-engine-xt-relationalStore-core-pure/src/main/resources/core_relational/relational/pureToSQLQuery/pureToSQLQuery.pure#L550
What type of PR is this?
Improvement
What does this PR do / why is it needed ?
Introduce new SetImplementation to support mapping functions returning Relations to Classes.
Which issue(s) this PR fixes:
Fixes #
Other notes for reviewers:
Does this PR introduce a user-facing change?