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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FE-7442] Sql group by having support #68

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@ghost
Copy link

commented Nov 19, 2018

Merge Checklist

馃敡 Issue(s) fixed:

  • Author referenced issue(s) fixed by this PR:
  • Fixes #0

馃毈 Smoke Test

  • Works in chrome
  • Works in firefox
  • Works in safari
  • Works in ie edge
  • Works in ie 11

馃殺 Merge

  • author crafted PR's title into release-worthy commit message.

Sachin Kaushik added some commits Jun 20, 2018

Sachin Kaushik
Sachin Kaushik
Support for SQL GROUP BY AND HAVING SUPPORT FOR ARRAY DATATYPE COLUMNS
Hi I have made changes in src/mapd-crossfilter.js file attached. Can you review my changes.

You can locate my changes by my name followed by my comments with start and end code comments

Please let me know your feedbacks on the same.

Setting example:
arrayDataConditions = [];

arrayDataConditions.push({
plainColumn: 'category',
column: 'unnest(category)',
operator: 'ILIKE',
value: "'%Restaurant%'",
});

cf.setArrayDataQueryParams([]);
cf.setArrayDataQueryParams(arrayDataConditions);

To view the result
cf.dimension('category').group().writeQuery()
@jrajav

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2018

@sachinkaushikHERE Please correct the formatting of mapd-crossfilter.js so it matches existing formatting and doesn't cause a diff over the entire file.

Sachin Kaushik
@ghost

This comment has been minimized.

Copy link
Author

commented Nov 20, 2018

@sachinkaushikHERE Please correct the formatting of mapd-crossfilter.js so it matches existing formatting and doesn't cause a diff over the entire file.

Done code Formatting

Please note the test are failing - TypeError: Cannot read property 'is_array' of undefined

I have rewritten getProjectOn method in group -> writeQuery method.

The issue was that in getProjectOn the unnest is concatenated if row index in dimArray matches with metadata row index order, which I see is ideal scenario Since I can add dimensions in any order to the crossfilter and it is by chance that my order of adding dimension column will match it in metadata row order if u can see cf.getColumns() rows sequence

Sachin Kaushik
Fixed bug related to duplicate Groups appearing in chart when there i鈥
鈥 more than one Group By on Array columns

@la-mapd la-mapd changed the title Sql group by having support [FE-7542] Sql group by having support Nov 27, 2018

@@ -6,7 +6,7 @@
"main": "./index.js",
"repository": {
"type": "git",
"url": "https://github.com/mapd/mapd-crossfilter.git"
"url": "https://github.com/sachinkaushikHERE/mapd-crossfilter.git"

This comment has been minimized.

Copy link
@arittr

arittr Nov 29, 2018

Contributor

let's make sure not to land this with this changed

This comment has been minimized.

Copy link
@ghost

ghost Nov 29, 2018

Author

Done

Sachin Kaushik
@jrajav

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

@sachinkaushikHERE I'm not sure how you did the code formatting, but can you try to match existing code formatting (that is, not change any lines not relevant to the actual changes)? Apologies that we don't have a canonical linter in this repo yet to make this simpler.

Also, having looked at the original ticket now, another request: Please add a unit test that generates an example query that is incorrect prior to this fix (for HAVING / ILIKE over array columns), but passes and generates the right query after it.

@jrajav

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

@sachinkaushikHERE Also, to confirm, is #59 rolled up in this PR as well?

@la-mapd la-mapd changed the title [FE-7542] Sql group by having support [FE-7442] Sql group by having support Nov 30, 2018

Sachin Kaushik
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.