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

Add RawSql tuple instances from 13 to 62 #1087

Merged
merged 8 commits into from
May 29, 2020
Merged

Add RawSql tuple instances from 13 to 62 #1087

merged 8 commits into from
May 29, 2020

Conversation

mitchellvitez
Copy link
Contributor

@mitchellvitez mitchellvitez commented May 27, 2020

See #1086. This adds up to the max length of a tuple for query results, so people will hopefully never run into result length limits.

If this formatting isn't optimal I can change it up, although I doubt there's going to be any way to format this kind of thing really nicely.

It could also admittedly be much nicer to do this with e.g. Template Haskell and I'd be happy to see this cleaned up using something like that.

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Bumped the version number if there isn't an (unreleased) on the Changelog
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

@mitchellvitez mitchellvitez changed the title Add RawSql tuple instances from 13 to 64 Add RawSql tuple instances from 13 to 62 May 27, 2020
@mitchellvitez
Copy link
Contributor Author

Here's the quick-and-dirty Python script I used to generate this, in case it's helpful

since = '-- | @since 2.11.0'
alphabet = list('abcdefghijklmnopqrstuvwxyz')

def cs(i):
    return (alphabet + [f'{c}2' for c in alphabet] + [f'{c}3' for c in alphabet])[:i]

def parens(s):
    return '(' + s + ')'

def raw_sql(i):
    return parens(', '.join([f'RawSql {c}' for c in cs(i)]))

def tuple(i):
    return parens(', '.join(cs(i)))

def compact_tuple(i):
    return parens(','.join(cs(i)))

def pairs(l):
    new = []
    for i in range(0, len(l)-1, 2):
        new.append(f'({l[i]},{l[i+1]})')
    if len(l) % 2:
        new.append(l[-1])
    return new

def paired_tuple(i):
    return parens(','.join(pairs(cs(i))))

for i in range(13, 63):
    print(f"""\
{since}
instance {raw_sql(i)}
      => RawSql {tuple(i)} where
    rawSqlCols e         = rawSqlCols e         . from{i}
    rawSqlColCountReason = rawSqlColCountReason . from{i}
    rawSqlProcessRow     = fmap to{i} . rawSqlProcessRow

{since}
from{i} :: {compact_tuple(i)} -> {paired_tuple(i)}
from{i} {compact_tuple(i)} = {paired_tuple(i)}

{since}
to{i} :: {paired_tuple(i)} -> {compact_tuple(i)}
to{i} {paired_tuple(i)} = {compact_tuple(i)}
""")

@MaxGabriel
Copy link
Member

Nice, this looks good to me. I'll leave this up for a couple days to see if anyone has any comments, but I think it's good to merge otherwise

Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, thanks so much!

@MaxGabriel
Copy link
Member

Update on this is that base doesn't support equality on tuples above size 15, so the test is failing https://www.stackage.org/haddock/lts-15.14/ghc-prim-0.5.3/GHC-Tuple.html#t:-40--44--44--44--44--44--44--44--44--44--44--44--44--44--44--44--41-

We're going to talk to @chessai about seeing if base could support equality up to 62

@parsonsmatt
Copy link
Collaborator

we're tuple pioneers

@mitchellvitez
Copy link
Contributor Author

I've changed the test here to only go up to 15-tuples. I'll try adding Eq/Ord instances to big tuples in base, then later can fix persistent to have up to 62

Not sure whether to leave the current 16-62 sized instances around in this PR. My instinct is it's still good to leave them in, because the type errors about lack of Eq are probably easier to understand than the ones about RawSql instances not existing

@chessai
Copy link

chessai commented May 27, 2020

So, Eq/Ord instances for larger tuples in base i'm on board with, but it of course shouldn't block this PR at all. base updates happen once every 6 months. i would just remove the tests for larger tuples, or derive Eq instances using StandaloneDeriving. The usage of tuples here "scales", so if a 15-tuple is correct, i would expect a 16-tuple to work as well, and then a 17-tuple, and then an 18-tuple, etc. Though there could be # some persistent-related issue im unaware of.

@MaxGabriel
Copy link
Member

persistent/ChangeLog.md Outdated Show resolved Hide resolved
mitchellvitez and others added 2 commits May 27, 2020 19:49
Co-authored-by: Maximilian Tagher <feedback.tagher@gmail.com>
Co-authored-by: Maximilian Tagher <feedback.tagher@gmail.com>
@MaxGabriel MaxGabriel merged commit 586cb81 into yesodweb:master May 29, 2020
@mitchellvitez mitchellvitez deleted the add-rawsql-instances branch May 29, 2020 17:03
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.

4 participants