feat: add main mathematical functions #3909

PrettyWood · 2023-12-09T17:02:56Z

With all the analytics DB, working on math and dates operations is key.
I will try to work on date / time / durations... types but still need some time for onboarding.
In the meantime if you're ok with this, I would like to first add math functions that are available for almost every dialects. In another PR I'll have a deeper look at string functions and I reckon it should cover pretty much #217 was opened for.
I'll work in a next PR on adding documentation for #1761 (similar to #1775)

PrettyWood · 2023-12-09T17:03:36Z

prqlc/prql-compiler/src/sql/std.sql.prql

+let asin = column -> s"ASIN({column:0})"
+let tan = column -> s"TAN({column:0})"
+let atan = column -> s"ATAN({column:0})"
+let pow = column exponent -> s"POW({column:0}, {exponent:0})"  # TODO: create an operator for that (** or ^ ?)


If you're ok with this PR, I reckon we should have some kind of operator for pow
Which one would you prefer?

Yes agree. No particularly strong view on ^ vs **, WDYT? (our parser can cope with the initial ambiguity of ** fine)

I personally would go with ** for 3 reasons

it's in python which is super used in data world (also in JS)

^ makes me think of bitwise XOR in Rust and Python

~~It feels more comfortable to type on a QWERTY ^^ I prefer to type twice a key than use shift~~ nvm I'm stupid

(Maybe ** uses up fewer unique characters, in case we want to use ^ for something else)

Edit: I just saw your reply, OK — let's go with **...

max-sixty

Super — this is v helpful, we should have done this a while ago.

They all look fine to me. I guess folks aren't using all of these (atan!) that much, but no harm in having them...

max-sixty · 2023-12-09T19:38:10Z

In another PR, we could add some of these to our integration tests. I think they're simple enough that I'm fairly confident they work, but nice to have coverage of them across dialects. A couple of times we've found inconsistencies between dialects this way.

If you also want to add them to the unit tests and/or docs, that would be good. Again, am quite confident they work, so not a top priority.

for more information, see https://pre-commit.ci

PrettyWood · 2023-12-09T23:43:22Z

@max-sixty I should be good to review :)

prqlc/prql-compiler/tests/integration/sql.rs

max-sixty · 2023-12-10T01:07:37Z

Great re adding ** here!

We need to set the associativity here:

prql/prqlc/prql-compiler/src/sql/gen_expr.rs

Lines 890 to 896 in de975cf

    
           fn associativity(&self) -> Associativity { 
        
               use BinaryOperator::*; 
        
               match self { 
        
                   Minus | Divide => Associativity::Left, 
        
                   _ => Associativity::Both, 
        
               } 
        
           }

It should be right associative (the only one). And we should add a test that the precedence works; 5 * 4 ** 3 ** 2 should be equivalent to 5 * (4 ** 9). (it's OK if it passes it through to SQL; it's important it doesn't insert parentheses that shouldn't be there)

If you prefer to push that to another PR, that's totally fine; we should just comment it out from the parser so it doesn't work yet.

CHANGELOG.md

PrettyWood · 2023-12-10T13:19:11Z

It should be right associative

You're completely right. I'll work on it tonight though I'll have to do some extra work because it's not a BinaryOperator (not available in SQL parser)

for more information, see https://pre-commit.ci

PrettyWood · 2023-12-10T17:43:09Z

@max-sixty So after some dive in I'm a bit perplex on how I should make the right associativity work.
What we want is for

c ** a ** b

to be translated into

POW(c, POW(a, b))

instead of

POW(POW(c, a), b)

Since there is no "power" operator in Postgres, we can't delegate that to the DB and there is nothing on sqlparser that handles that directly. I tried to use BinaryOperator::Custom(String::from("**")) for example and set right associativity on it, it doesn't work.
IMO what we want is for c ** a ** b to be translated into (c ** a) ** b before that but I would need some guidance 😬

max-sixty · 2023-12-10T18:43:10Z

OK, good point... this is actually a bit harder than I thought — it's not only adjusting the SQL writer, it's also adjusting the parser here:

prql/prqlc/prqlc-parser/src/expr.rs

Line 250 in 054d29e

.foldl(|left, (op, right)| {

, I think.

(Previously everything had been left-associative, so all the work I'd done here had been on the SQL writer)

I can come back to this and look more. In the meantime, can we comment out the change in the parser so ** doesn't work yet? We can leave the rest of the code there so it'll flow through when we do fix it.

prqlc/prqlc-parser/src/lexer.rs

web/book/src/reference/syntax/operators.md

max-sixty · 2023-12-10T18:47:57Z

grammars/prql-lezer/src/prql.grammar

@@ -165,7 +165,7 @@ LambdaParam { identPart TypeDefinition? (":" expression)? }
  Comment { "#" ![\n]* }
  @precedence { Docblock, Comment }

-  //op_bin_only { "*" | "/" | "//" | "%" | "!=" | ">=" | "<=" | "~=" | ">" | "<" | "??" | "&&" | "||" }
+  //op_bin_only { "*" | "/" | "//" | "%" | "**" | "!=" | ">=" | "<=" | "~=" | ">" | "<" | "??" | "&&" | "||" }


@vanillajonathan should we remove this line?

Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>

for more information, see https://pre-commit.ci

PrettyWood · 2023-12-10T19:29:33Z

Ok I removed the ** operator I think we're good!
One last remark: should I write pow as pow = exponent column instead of pow = column exponent ?
I'm a bit torn between:

SQL that uses POW(col, 2)
PRQL where I would prefer to write derive { pow_col = col | pow 2 }

max-sixty · 2023-12-10T19:41:23Z

PRQL where I would prefer to write derive { pow_col = col | pow 2 }

I think this is correct — the col generally goes last, because piping puts the argument last, and is the thing that's normally piped.

We do something similar for round etc...

prqlc/prql-compiler/src/sql/std.sql.prql

vanillajonathan · 2023-12-10T23:36:32Z

It would be pretty convenient if all the math functions could be grouped together under a math namespace.

Like Math in JavaScript.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math

PrettyWood · 2023-12-10T23:42:42Z

Yup @vanillajonathan I thought about it and agree with you I'd like that. We could have the same for string utils.
This would be a bit more explicit and avoid name collisions

max-sixty · 2023-12-11T03:58:29Z

Yes, I agree...

feat: add main mathemtical functions

8a18d74

PrettyWood changed the title ~~feat: add main mathemtical functions~~ feat: add main mathematical functions Dec 9, 2023

PrettyWood commented Dec 9, 2023

View reviewed changes

max-sixty approved these changes Dec 9, 2023

View reviewed changes

PrettyWood added 4 commits December 9, 2023 20:43

feat: add definitions

89775ff

feat: add highlight

a8bba87

feat: support new ** as pow operator

3256f43

test: add basic TU

3e71c64

PrettyWood marked this pull request as ready for review December 9, 2023 23:22

PrettyWood force-pushed the math-functions branch from 7a2349c to 6d2ea0f Compare December 9, 2023 23:36

PrettyWood and others added 3 commits December 10, 2023 00:42

test: update snapshots

8f846f8

chore: changelog

0c72fa4

[pre-commit.ci] auto fixes from pre-commit.com hooks

fa25dad

for more information, see https://pre-commit.ci

PrettyWood force-pushed the math-functions branch from d1899a7 to fa25dad Compare December 9, 2023 23:42

max-sixty reviewed Dec 10, 2023

View reviewed changes

prqlc/prql-compiler/tests/integration/sql.rs Show resolved Hide resolved

max-sixty reviewed Dec 10, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

PrettyWood and others added 4 commits December 10, 2023 16:46

chore: clean line break

1659506

chore: some fixes

054d29e

[pre-commit.ci] auto fixes from pre-commit.com hooks

f7d9549

for more information, see https://pre-commit.ci

Merge branch 'main' into math-functions

2849bb1

test: add fixme note

e411f05

max-sixty reviewed Dec 10, 2023

View reviewed changes

prqlc/prqlc-parser/src/lexer.rs Outdated Show resolved Hide resolved

max-sixty reviewed Dec 10, 2023

View reviewed changes

web/book/src/reference/syntax/operators.md Outdated Show resolved Hide resolved

max-sixty reviewed Dec 10, 2023

View reviewed changes

max-sixty approved these changes Dec 10, 2023

View reviewed changes

PrettyWood and others added 3 commits December 10, 2023 20:04

Update web/book/src/reference/syntax/operators.md

5c8ceb9

Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

1a2625f

for more information, see https://pre-commit.ci

remove ** operator for now

bd47559

max-sixty reviewed Dec 10, 2023

View reviewed changes

prqlc/prql-compiler/src/sql/std.sql.prql Show resolved Hide resolved

refactor: switch exponent to first arg in pow

a41b2a7

PrettyWood merged commit 41dbf82 into main Dec 10, 2023
34 checks passed

PrettyWood deleted the math-functions branch December 10, 2023 19:51

PrettyWood mentioned this pull request Dec 10, 2023

fix: pow for mssql #3911

Merged

eitsupi mentioned this pull request Jan 10, 2024

** operator (or something else) for math.pow #4065

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add main mathematical functions #3909

feat: add main mathematical functions #3909

PrettyWood commented Dec 9, 2023 •

edited

Loading

PrettyWood Dec 9, 2023

max-sixty Dec 9, 2023

PrettyWood Dec 9, 2023 •

edited

Loading

max-sixty Dec 9, 2023 •

edited

Loading

max-sixty left a comment

max-sixty commented Dec 9, 2023

PrettyWood commented Dec 9, 2023

max-sixty commented Dec 10, 2023

PrettyWood commented Dec 10, 2023

PrettyWood commented Dec 10, 2023

max-sixty commented Dec 10, 2023

max-sixty Dec 10, 2023

PrettyWood commented Dec 10, 2023 •

edited

Loading

max-sixty commented Dec 10, 2023

vanillajonathan commented Dec 10, 2023

PrettyWood commented Dec 10, 2023

max-sixty commented Dec 11, 2023

feat: add main mathematical functions #3909

feat: add main mathematical functions #3909

Conversation

PrettyWood commented Dec 9, 2023 • edited Loading

PrettyWood Dec 9, 2023

Choose a reason for hiding this comment

max-sixty Dec 9, 2023

Choose a reason for hiding this comment

PrettyWood Dec 9, 2023 • edited Loading

Choose a reason for hiding this comment

max-sixty Dec 9, 2023 • edited Loading

Choose a reason for hiding this comment

max-sixty left a comment

Choose a reason for hiding this comment

max-sixty commented Dec 9, 2023

PrettyWood commented Dec 9, 2023

max-sixty commented Dec 10, 2023

PrettyWood commented Dec 10, 2023

PrettyWood commented Dec 10, 2023

max-sixty commented Dec 10, 2023

max-sixty Dec 10, 2023

Choose a reason for hiding this comment

PrettyWood commented Dec 10, 2023 • edited Loading

max-sixty commented Dec 10, 2023

vanillajonathan commented Dec 10, 2023

PrettyWood commented Dec 10, 2023

max-sixty commented Dec 11, 2023

PrettyWood commented Dec 9, 2023 •

edited

Loading

PrettyWood Dec 9, 2023 •

edited

Loading

max-sixty Dec 9, 2023 •

edited

Loading

PrettyWood commented Dec 10, 2023 •

edited

Loading