Skip to content
This repository has been archived by the owner on Jan 28, 2021. It is now read-only.

add greatest and least #700

Merged
merged 7 commits into from
May 20, 2019
Merged

add greatest and least #700

merged 7 commits into from
May 20, 2019

Conversation

juanjux
Copy link
Contributor

@juanjux juanjux commented May 3, 2019

  • Adds MySQL GREATEST and LEAST functions.
  • Sort function names in SUPPORTED.md.

Signed-off-by: Juanjo Alvarez juanjo@sourced.tech

@juanjux juanjux self-assigned this May 3, 2019
README.md Show resolved Hide resolved
sql/expression/function/greatest_least.go Outdated Show resolved Hide resolved
[]sql.Row{{int64(4)}},
},
{
`SELECT LEAST(1, 2, 3, 4)`,
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add some tests using strings too?

Copy link
Contributor

Choose a reason for hiding this comment

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

And some tests using table columns too, please

Copy link
Contributor Author

@juanjux juanjux May 6, 2019

Choose a reason for hiding this comment

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

@ajnavarro there are already tests with strings on greatest_latest_test.go (the ones called "all strings", "all strings and empty", and a couple mixing strings with other types).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ajnavarro , @erizocosmico added tests with strings and tables to this file, PTAL.

sql/expression/function/greatest_least.go Outdated Show resolved Hide resolved
sql/expression/function/greatest_least.go Outdated Show resolved Hide resolved
sql/expression/function/greatest_least.go Outdated Show resolved Hide resolved
sql/expression/function/greatest_least.go Outdated Show resolved Hide resolved
sql/expression/function/greatest_least.go Outdated Show resolved Hide resolved
sql/expression/function/greatest_least_test.go Outdated Show resolved Hide resolved
sql/expression/function/greatest_least_test.go Outdated Show resolved Hide resolved
@juanjux juanjux force-pushed the greatest_least branch 3 times, most recently from 6de29fc to c20ec39 Compare May 6, 2019 09:22
@src-d src-d deleted a comment from kuba-- May 6, 2019
@juanjux juanjux force-pushed the greatest_least branch 3 times, most recently from b8660ef to df87b11 Compare May 7, 2019 09:44
@juanjux
Copy link
Contributor Author

juanjux commented May 7, 2019

Return type deduction implemented as suggested plus some factorizing for NewXXX and Eval to avoid duplicated code. Only missing the tests with a table and columns (currently checking how to do that :)

sql/expression/function/greatest_least.go Outdated Show resolved Hide resolved
sql/expression/function/greatest_least_test.go Outdated Show resolved Hide resolved
}

switch t := val.(type) {
case int, int32, int64:
Copy link
Contributor

Choose a reason for hiding this comment

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

What about unsigned variants?
I don't want to be too much tiquismiquis, but maybe we should stick just to 2 options - string or float64.
I remember we had the same pleasure fighting with different types for arithmetic functions (max, min, avg, ...) and ultimately we decided to stick to float64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, there is not much difference between supporting only string and float64 and also supporting int/32/64 but not unsigned.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just meant:

if string {
// convert to string
} else {
// convert to float64
}

if we wanna keep the current implementation it's fine, if we add int8, int16 and unsigned variants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ajnavarro
Copy link
Contributor

@juanjux LGTM after resolving conflicts!

Juanjo Alvarez added 7 commits May 20, 2019 10:38
typo fix

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@juanjux
Copy link
Contributor Author

juanjux commented May 20, 2019

@ajnavarro done!

@ajnavarro ajnavarro merged commit b8dcf43 into src-d:master May 20, 2019
@juanjux juanjux mentioned this pull request May 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants