Make all columns be filterable or groupable #6764
Conversation
agrawaldevesh
force-pushed
the
all_filt_group
branch
from
7302b96
to
368a5c1
Compare
Codecov Report
@@ Coverage Diff @@
## master #6764 +/- ##
=======================================
Coverage 56.07% 56.07%
=======================================
Files 526 526
Lines 23257 23257
Branches 2782 2782
=======================================
Hits 13042 13042
Misses 9806 9806
Partials 409 409
Continue to review full report at Codecov.
|
agrawaldevesh
force-pushed
the
all_filt_group
branch
2 times, most recently
from
4250434
to
b21c9e5
Compare
superset/config.py
Outdated
| @@ -188,7 +188,9 @@ | |||
| # --------------------------------------------------- | |||
| # Feature flags that are on by default go here. Their | |||
| # values can be overridden by those in super_config.py | |||
| FEATURE_FLAGS = {} | |||
| FEATURE_FLAGS = { | |||
| 'ALL_COLS_FILTERABLE_GROUPABLE': False, | |||
There was a problem hiding this comment.
@xtinec , Can you review this use of feature flags ?
There was a problem hiding this comment.
I'm not sure how these feature flags are supposed to work in practice in superset_config. It appears there's no good way to alter only one of them. You'd have to either alter all of them or none of them. If you were to copy/paste the whole dict to your superset_config you'd miss out on new ones as they come, and if you try to import the dict I think you'd run in circular import issue. @xtinec what do you think?
There was a problem hiding this comment.
@agrawaldevesh let's figure out what's up with postgres, let's not use the feature flag for this specifically or at least until we confirm it's usable. If you repush without the flag I can try and help debug the postgres-specific issue
|
Personally would push for this being the new default, no need for a feature flag |
mistercrunch
added
enhancement:request
It breaks the postgres unit tests, but I couldn't figure out why. Its something related to timeseries and such. Funnily, sqlite and mysql tests pass with this. |
agrawaldevesh
force-pushed
the
all_filt_group
branch
2 times, most recently
from
400621b
to
6ad1833
Compare
|
@mistercrunch, I tried setting the feature flag as default true and it fails the mysql or pg unit tests in https://travis-ci.org/apache/incubator-superset/jobs/485991707 (in tests.sqllab_tests.SqlLabTests#test_search_query_on_time). I frankly cannot comprehend these tests: They seem to be doing some meta level searching of the query log themselves. I am not sure why other tests in this file are succeeding with the NO_TABLE table but some fail. Perhaps its to do with some inadvertent state changes, of tests interferring with each other or not cleaning up properly. Anyway, I think we can keep this flag be FALSE for now and set it to true/default after a while. |
agrawaldevesh
force-pushed
the
all_filt_group
branch
from
6ad1833
to
3e34d5b
Compare
|
@mistercrunch, sure ... it would be best to fix the root test issue :-). I think its a bit flaky. I have seen this fail with mysql once, once with postgres and once all tests passed. And now I am running the tests again: https://travis-ci.org/agrawaldevesh/incubator-superset/builds/486245360 |
agrawaldevesh
force-pushed
the
all_filt_group
branch
from
3e34d5b
to
0a2767c
Compare
|
Sounds good. Let's get to the bottom of it. |
This is not configurable and will be the default (and only !) behavior going forward !
agrawaldevesh
force-pushed
the
all_filt_group
branch
from
0a2767c
to
3dcb575
Compare
|
@mistercrunch, as suggested, I have made this be the brave new default :-) |
agrawaldevesh
changed the title
|
LGTM |
mistercrunch
added
🏷️ bot
I want to make every column be filterable and groupable and not just strings.