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

sum() doesn't work for non-number columns #200

Open
cakoose opened this issue Dec 29, 2020 · 4 comments
Open

sum() doesn't work for non-number columns #200

cakoose opened this issue Dec 29, 2020 · 4 comments

Comments

@cakoose
Copy link
Collaborator

cakoose commented Dec 29, 2020

In my Mammoth schema definition, I'm using big.js for Postgres numeric and JS BigInt for Postgres int8.

The Mammoth sum function doesn't work on those types:

sum: (expression: Expression<number, boolean, any>) => Expression<number, false, "sum">;

The quick fix for me is to define my own sum function with a different type. That's what I'm going to do for now.

But I wonder if there's a more general solution. One option would be to add a new "IsSummable" type parameter to the Expression type. But another type parameter ends up adding a bunch of noise to the IDE auto-complete and error messages.

I think Scala "type members" (called "associated types" in Rust and Swift) might provide a cleaner solution, but I'm not sure if that's possible in TypeScript. Here's one attempt to simulate them in TypeScript: StackOverflow link.

@cakoose
Copy link
Collaborator Author

cakoose commented Dec 29, 2020

BTW, this is the workaround I'm using:

import * as mammoth from '@ff00ff/mammoth';

export function sum<T extends BigInt | number | Big>(expression: Expression<T, boolean, any>): Expression<T, true, 'sum'> {
    const casted = expression as Expression<number, boolean, any>;
    return mammoth.sum(casted);
}

Edit: fixed return value from Expression<number, boolean, 'sum'> to Expression<T, false, 'sum'>.

@martijndeh
Copy link
Contributor

martijndeh commented Dec 30, 2020

I think there should be a Number type which can be changed in user-land. Either by passing it to the db (but this requires always importing these function through your db instance) or change the Mammoth declarations in user-land.


So something like type MammothNumber = number; which will be used by sum and all those number operations and it can be altered to e.g. MammothNumber = number | BigInt | Big in user-land.

@martijndeh
Copy link
Contributor

Somewhat related: I think Mammoth must make a distinction between int8 and int4 in the public interface. In the case of sum() this is probably fine, but when you do math like int8 + int4 we should make sure the correct type is returned (in that case int8). If we just have one type for all numeric types we cannot make the distinction and this must be asserted in user-land.

@martijndeh
Copy link
Contributor

Hmm I thought I had a fix but it's not working properly yet.

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

No branches or pull requests

2 participants