Skip to content

fix QUART/MEDIAN/MIN/MAX on number/date/string - #1156

Merged
mathiasrw merged 3 commits into
AlaSQL:developfrom
forestfang-stripe:fix-quartile-comparator
Jan 17, 2020
Merged

mathiasrw merged 3 commits into
AlaSQL:developfrom
forestfang-stripe:fix-quartile-comparator

Conversation

@forestfang-stripe

Copy link
Copy Markdown
Contributor

Fix MIN/MAX and GREATEST/LEAST to work on date and string by actively compare with < and > instead of Math.min and Math.max. This is commonly supported by other SQL engine: https://www.w3resource.com/sql/aggregate-functions/min-function.php
Currently, MIN/MAX works for Date but the data type changes to number, which is not what user would expect.

Fix QUART/MEDIAN to use custom comparator with < and >. When comparator is not supplied, Javascript coerce arguments to string:

If omitted, the array elements are converted to strings, then sorted according to each character's Unicode code point value.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort

Enabled skipped tests on these functions and update tests for new and correct behaviors (for example, median of numbers between 1 and 10000 should be 5000 not 5499.) add test from #104

p.s. some files got auto-formatted, specifically for 423groupby.js, I only changed 'MIN' and 'MAX' implementation.

closes #411

Comment thread src/423groupby.js
colexp +
');' +
post
pre + 'if ((y=' + colexp + ") > g['" + colas + "']) g['" + colas + "'] = y;" + post

@forestfang-stripe forestfang-stripe Jan 16, 2020 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I used y here assuming it is intended to be a temporary variable (as far as I can tell from how it is used in AVG) so colexp don't get evaluated twice.

@mathiasrw

Copy link
Copy Markdown
Member

You are soooo deep into the details. Wow. I love it. even a nice test case. Perfect.

@mathiasrw
mathiasrw merged commit 3a77eed into AlaSQL:develop Jan 17, 2020
@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #1156 into develop will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1156      +/-   ##
===========================================
- Coverage    41.53%   41.53%   -0.01%     
===========================================
  Files            2        2              
  Lines        21012    21004       -8     
  Branches      6295     6291       -4     
===========================================
- Hits          8727     8723       -4     
+ Misses       12285    12281       -4
Impacted Files Coverage Δ
dist/alasql.fs.js 67.54% <0%> (-0.02%) ⬇️
dist/alasql.js 15.21% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbbaf25...e62cfac. Read the comment docs.

@mathiasrw

Copy link
Copy Markdown
Member

Released as version 0.5.4 💟

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.

MAX/MIN function not working for words

3 participants