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

Correct nullability for set operations #18419

Merged
merged 1 commit into from
Oct 17, 2019
Merged

Conversation

roji
Copy link
Member

@roji roji commented Oct 17, 2019

Fixes #18135

@smitpatel some minor comments:

  • Technically we should also make the outer EntityProjectionExpression itself nullable when one of the inner ones is nullable, but we currently can't check if an EntityProjectionExpression is nullable or not - we could expose IsNullable for this purpose. This doesn't seem to matter in practice since EntityProjectionExpression's nullability is only consulted in BindProperty, but for set operations _propertyExpressionsCache is already populated correctly. Let me know if you want me to do this and expose IsNullable.
  • In a perfect world, isNullableProjection should drill down into the expression to really understand whether it's nullable or not. Currently we just assume nullable except for some very basic cases.
  • I currently duplicated isNullableProjection from ColumnExpression, we should DRY this at some point.

@roji roji requested a review from smitpatel October 17, 2019 10:49
@roji roji force-pushed the SetOperationNullability branch 2 times, most recently from d3738d3 to 1fcecd3 Compare October 17, 2019 11:57
@smitpatel
Copy link
Member

EntityProjection is marked as nullable only if it is mapped to a table. The source table can project out nulls so when generating ColumnExpression make them nullable. If the entityProjection is not mapped to a table then nullable flag is unnecessary. The backing propertyExpressions should already be nullable if required. May be we could decorate TableExpressionBase as nullable but breaking change.

  1. So no, we are not exposing EntityProjection.IsNullable or marking EntityProjection as nullable when passing propertyExpressions.
  2. Low value, Some perf benefit.
  3. May be.

@roji
Copy link
Member Author

roji commented Oct 17, 2019

The backing propertyExpressions should already be nullable if required

Yeah, I see that - just a bit uncomfortable about having EntityProjection lying about its nullability, in case we start looking at it in some place in the future etc.

@smitpatel
Copy link
Member

EntityProjection does not lie about anything because it does not tell you its nullability.

@roji
Copy link
Member Author

roji commented Oct 17, 2019

EntityProjection does not lie about anything because it does not tell you its nullability.

Right, but it contains an incorrect setting about itself, in case we ever need to rebind a property or similar. I understand it's irrelevant in the current code/design though.

@smitpatel
Copy link
Member

What is rebinding a property? If you pass a table then tell if it can be null. Purely metadata to create columnExpression if not created already. There is no setting which is incorrect.

@roji
Copy link
Member Author

roji commented Oct 17, 2019

Never mind, am just leaving it as-is.

@smitpatel
Copy link
Member

If you think there is a bug or design issue somewhere then please explain in detail what scenario is not working or why it is an issue.

cc: @ajcvickers

@roji
Copy link
Member Author

roji commented Oct 17, 2019

Apologies, was not suggesting there was a bug in the design or anything... There's no bug here, just a possible matter of future-proofing it by making sure that EntityProjections always has the correct _nullable value internally, which I wanted to signal. It's valid to skip this if you don't think it's worthwhile.

@roji roji merged commit 75a5f2e into release/3.1 Oct 17, 2019
@roji roji deleted the SetOperationNullability branch October 17, 2019 19:24
roji added a commit that referenced this pull request Oct 22, 2019
Make async and simplify.

Continues #18419
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.

Set operations need to consider nullability of columns on both sides
2 participants