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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

basic elastic connector implementation #5358

Closed
wants to merge 12 commits into from

Conversation

takkarharsh
Copy link

@takkarharsh takkarharsh commented Jul 6, 2018

I tried running the code for earlier PR 2953 raised for elastic search , but i was not able to run it due to some package issue,
I have updated the code and , now it is able to connect to elastic search , however ui view and agg function needs to be corrected further

upgrade steps
cd $SUPERSET_HOME
pip install . --upgrade
superset db upgrade
superset init

@takkarharsh takkarharsh changed the title basic elastic connector implementations basic elastic connector implementation Jul 6, 2018
@mistercrunch
Copy link
Member

Can you keep my commit(s) intact so we can identify what's been put on top easily and keep have the history attributed to the right contributor.

@takkarharsh
Copy link
Author

@mistercrunch I have cherry picked your changes and again merged the change, Thanks for pointing it.

@takkarharsh
Copy link
Author

@mistercrunch Please have a look for the validity of implementation, if it looks good, let us know the further steps to fix the integration issues

base_order = ('datasource_name', 'asc')
description_columns = {
'slices': _(
'The list of slices associated with this table. By '
Copy link
Member

Choose a reason for hiding this comment

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

We don't use the term "slice" anymore and use "chart" instead. We haven't remove slice references to object names and such, but from all user-facing terminology

@mistercrunch
Copy link
Member

That's generally the right direction. I think the though part will be around filling up the methods in models.py

@codecov-io
Copy link

codecov-io commented Jul 20, 2018

Codecov Report

Merging #5358 into master will increase coverage by 10.6%.
The diff coverage is 48.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5358      +/-   ##
==========================================
+ Coverage   48.34%   58.95%   +10.6%     
==========================================
  Files         328      375      +47     
  Lines       14762    24153    +9391     
  Branches     2758     2758              
==========================================
+ Hits         7137    14239    +7102     
- Misses       7610     9899    +2289     
  Partials       15       15
Impacted Files Coverage Δ
superset/config.py 92.53% <ø> (ø)
superset/connectors/elastic/__init__.py 100% <100%> (ø)
superset/connectors/druid/views.py 68.45% <100%> (ø)
superset/connectors/elastic/models.py 38.9% <38.9%> (ø)
superset/connectors/elastic/views.py 70.79% <70.79%> (ø)
superset/views/core.py 73.37% <0%> (ø)
superset/data/countries.py 100% <0%> (ø)
superset/models/annotations.py 92.85% <0%> (ø)
superset/views/annotations.py 67.64% <0%> (ø)
... and 42 more

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 7fcc2af...7de8e1a. Read the comment docs.

@bipinsoniguavus
Copy link
Contributor

@mistercrunch we are getting the following error Multiple head revisions are present for given argument 'head'; please specify a specific target revision, '<branchname>@head' to narrow to a specific head, or 'heads' for all heads, could you please guide us why it happens everytime when we commit our code? we are fixing it by running superset db merge heads to resolve this problem, but we don't want to do on each commit. Please suggest the proper way to resolve it.

@takkarharsh
Copy link
Author

Hi @mistercrunch, Please let us know the correct way to merge head as , every time there is a new checkin in master we get multiple head issue

@TimHeckel
Copy link

The requirements.txt file lists 5.5.3 as the minimum version for elasticsearch. Official SQL support landed in elasticsearch as of 6.3.0. It seems like this PR is still predicated on 5.5.3? Much like its predecessor? With perhaps an attempt to map the SQL coming in the SQL alchemy api without relying on the new sql _translate endpoint? If that's the intent here, I think it would be smart to focus solely on ES 6.3.0+ -- am I understanding this correctly?

@stale
Copy link

stale bot commented Apr 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the inactive Inactive for >= 30 days label Apr 10, 2019
@stale stale bot closed this Apr 17, 2019
@mistercrunch
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Inactive for >= 30 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants