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

HasField for SqlExpr (Maybe (Entity a)) joins Maybe #422

Merged
merged 8 commits into from
Jan 7, 2025

Conversation

parsonsmatt
Copy link
Collaborator

@parsonsmatt parsonsmatt commented Dec 27, 2024

This may make type inference really bad, so I want to do a lot more experimentation with it before committing to this. The convenience would be very nice but not if the error messages get really bad.

Closes #407

Might as well fold a few more items into this PR. Other functions which produce SqlExpr (Value (Maybe a)) should probably do Nullable here also.

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR.
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts).

@parsonsmatt parsonsmatt changed the base branch from master to v3.6.0.0 December 27, 2024 21:17
@parsonsmatt
Copy link
Collaborator Author

OK, I've been playing with this on the work codebase. I have some observations.

The Question of ?. and OverloadedLabels

foo.bar doing nullable field projection actually works really great. I am really not entirely sure why it works so well, compared to foo ?. #bar, which has lots of weird type inference issues. But foo ?. FooBar continues to work great with nullable field projection.

My guess is that the (SymbolToField sym rec typ) => IsLabel sym (EntityField rec typ) instance just doesn't bring enough constraints or power to bear, and the inference falls apart. So foo ?. #bar form becomes pretty much useless from a type inference perspsective. This is... unfortunate.

So, should ?. be changed? If so, to use Nullable or NullableFieldProjection?

How many other functions should do this?

subSelect :: SqlQuery (SqlExpr (Value a)) -> SqlExpr (Value (Maybe a)) is a clear possibility, as are all the aggregation functions. But this may cause type inference issues.

just and joinV - to make polymorphic or not?

If we make joinV and just polymorphic, then we can avoid changing a lot more code. But we also risk worse type inference.

I suppose just' and joinV' which retain the old fixed behavior may be a good compromise here.

Should they use Nullable or NullableFieldProjection?

And when? What are the differences? They seem very similar in terms of functionality, but the inference properties are different - how?

Lots of questions.

@parsonsmatt
Copy link
Collaborator Author

parsonsmatt commented Jan 3, 2025

On ?.

If we make ?. polymorphic, then we introduce type inference problems... but honestly, very few.

Making it return SqlExpr (Value (Maybe (Nullable a))) actually works pretty well. If you write polymorphic code in ?., then you'll have to propagate a constraint. But this is relatively rare in the Mercury codebase.

Weirdly, using the type class causes significant inference problems.

Since both have some Issues, I am going to create an operator ??. which retains the original type behavior, Just In Case.

On just

Even with polymorphic projection, I find that a lot of just uses are complaining about Maybe. With just :: (NullableFieldProjection typ typ') => SqlExpr (Value typ) -> SqlExpr (Value (Maybe typ')) we only really save ourselves from type-errors at just (just (val x)) or just (just foo.bar) uses.

With a type family:

type SqlMaybe a where
    SqlMaybe (Maybe a) = Maybe a
    SqlMaybe a = Maybe a

This should do something similar, but it ends up being really awkward, as just is often used to produce polymorphic values- ie just (val 0) - and this ends up having awful inference properties. Propagating this type family around causes a significant amount of noise, too, so it's probably best to skip that strategy.

Weirdly, just foo.maybeColumn ==. maybeTable.column gives a type error with both monomorphic just and polymorphic just. And the error message is slightly worse with polymorphic just:

    • Couldn't match type: Maybe typ'
                     with: Key ExternalOnboardingData
        arising from a functional dependency between:
          constraint ‘GHC.Records.HasField
                        "id"
                        (SqlExpr (Maybe (Entity ExternalOnboardingData)))
                        (SqlExpr (Value (Key ExternalOnboardingData)))’
            arising from selecting the field ‘id’
          instance ‘GHC.Records.HasField
                      sym (SqlExpr (Maybe (Entity rec))) (SqlExpr (Value (Maybe typ'1)))’
            at <no location info>

compared to -- ah, well, alas. Actually the just foo.bar ==. mfoo.col has the same type error information, because it's coming from the mfoo.col rather than just. So even monomorphic just has some bad error messages. It's just that the polymorphic just doesn't have the errors.

@parsonsmatt
Copy link
Collaborator Author

OK, I'm pretty happy with the PR in this state. I've tested this out on Mercury's codebase and it's not a huge change, but it's a very good change.

@parsonsmatt parsonsmatt marked this pull request as ready for review January 7, 2025 16:43
@parsonsmatt parsonsmatt merged commit 1010b1e into v3.6.0.0 Jan 7, 2025
9 checks passed
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.

HasField for SqlExpr (Maybe (Entity a)) should use Nullable
1 participant