Added metadata for complextypes for AddViews / TVF in RelationalModel

[kirolous-nashaat]

• Added metadata for complextypes for AddViews in RelationalModel
• Added test for select complex property from tvf

Issue #34627

• I've read the guidelines for contributing and seen the walkthrough
• I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
• The code builds and tests pass locally (also verified by our automated build checks)
• Commit messages follow this format
• Tests for the changes have been added (for bug fixes / features)
• Code follows the same patterns and style as existing code in this repo

[kirolous-nashaat]

@dotnet-policy-service agree

[SamMonoRT]

There are failing tests, please address those

[AndriySvyryd]

@roji What do you think about this?

[roji]

Thanks for the ping.

Yeah, I don't we should be doing this - not in this PR, in any case. We don't currently flow "for query" vs "for update" into the query pipeline for any other case; our current approach to this is instead to rewrite the main target table from the view to the table, specifically when translating ExecuteUpdate (see code). The advantage of that approach is keeping the view vs. table concern out of the query pipeline, and dealing with it only where it's relevant.

So the existing code (linked to above) should take care of everything (including for complex properties) - in fact I'm surprised it does not do so already. What exactly prompted you to introduce this change?

I'm also noting that we should only be referencing the table for the mutation; any property accesses elsewhere (e.g. in the WHERE clause) should reference the view.

If we do want to change the way we're dealing with this and filter the information into the query pipeline, we should probably do this more generally in a separate PR, and remove the current code switching from the view to the table which I linked to above.

[kirolous-nashaat]

please check this comment for initial thoughts about the issue
#34627 (comment)

before the change, we didnt have view metadata for complex types

the test mentioned above broke cuz of this change
it was relying on complex props not having mapping in the view ( container expression ) , it throws key not found , as expected in the test

after the change
now we have metadata for complex types, execute update find a mapping in the view
try to execute something like this
update viewx set column = ..

which throws exception cuz viewx isnot a table and can't be updated

so to keep the old test behavior and not break it, i had to know if i need table mappings only or viewortablemappings

select expression mapping of complex property does use getviewortablemapping
which return = view ?? table
so you need to distinguish if you need select expression for update / read statements accordingly

[kirolous-nashaat]

I'm also noting that we should only be referencing the table for the mutation; any property accesses elsewhere (e.g. in the WHERE clause) should reference the view.

not sure about this part.
for example i want to update table where a column is x

there is a where condition but because we are in an update statement we cant use the view at all

so mutation uses table regardless of where/set

[roji]

after the change
now we have metadata for complex types, execute update find a mapping in the view
try to execute something like this
update viewx set column = ..

So as I wrote above, we already have code in the ExecuteUpdate translation which is supposed to take care of this - I'm wondering what it doesn't work.

Note that I'm not necessarily opposed to the approach here: flowing forUpdate in like this PR proposes is in some ways probably better than the current approach of doing a translation, and then rewriting that translation to transform the view to the table (always better do just do the right thing immediately rather than do one thing and rewrite later, and this would certainly simplify the ExecuteUpdate logic by removing the transformation...).

But if we do go in this direction, we should switch to it fully, rather than doing one thing for complex properties and another for regular entities.

@AndriySvyryd any thoughts?

[kirolous-nashaat]

so @roji , you mean i should try to make the execute update work or do we still need it to fail in case of complex types + view + table ?

by figuring out why the snippet you sent didn't work as expected

[roji]

ExecuteUpdate definitely shouldn't fail for complex types, though we can also decide to handle that separately if we want - it's OK for this PR to just handle the metadata side, and then a separate issue/PR for making sure everything works in the query pipeline.

But in general, yes, we should figure out why the existing code (which I sent) doesn't take care of things. As an alternative, we could decide that your way (flowing forUpdate into the query pipeline) is better, but then we need everything to work that way - include non-complex (entity) types. Your PR, as is, would mean that there are two different approaches to the same problem: for regular entity types, the code snippet I sent handles this in ExecuteUpdate (by rewriting the view to a table), whereas for complex types, we flow forUpdate in. We shouldn't have two ways of doing this.

Am mainly wanting to hear @AndriySvyryd's opinion on this.