Skip to content

Conversation

@eddies
Copy link

@eddies eddies commented Feb 19, 2016

What is this PR for?

An update/follow-on for PR 152 that is limited to Leaflet support. There doesn't appear to have been any new movement on PR 152 since August 2015, so this PR updates @Madhuka's work against master, but without any of the data validation or other non-Leaflet work.

What type of PR is it?

Feature

Todos

N/A

Is there a relevant Jira issue?

ZEPPELIN-157

How should this be tested?

Recreate @Madhuka's example, e.g. use the following to populate a cell:
https://gist.github.com/eddies/f20241e161aa2bbbb788

Then, in a new cell

%sql
select * from myMap

There should a new chart selector rightmost that will render the data w/ leaflet. You may need to clean your browser cache initially.

Screenshots (if appropriate)

leaflet
Also see Madhuka's original PR: #152

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? Probably. It wasn't clear to me if PR 152 was going to move ahead in a new, Leaflet-only incarnation as suggested in the comment thread. But I needed Leaflet support, so went ahead with my own fork. I'm happy to work on documentation for this, but not sure if a sample notebook tutorial is what's desired (e.g. maybe something under the Display System docs is preferable?). If so, I'd just stick with something like the sample notebook in PR 152.

@eddies eddies changed the title Leaflet support, based PR 152 Leaflet support, based on PR #152 Feb 19, 2016
@eddies
Copy link
Author

eddies commented Mar 1, 2016

FYI, 18b4f55 is failing CI with the following:

[ERROR] bower leaflet.heat#v0.2.0                        ECMDERR Failed to execute "git ls-remote --tags --heads [email protected]:Leaflet/Leaflet.heat.git", exit code of #128 Warning: Permanently added the RSA host key for IP address '192.30.252.131' to the list of known hosts. Permission denied (publickey). fatal: Could not read from remote repository.  Please make sure you have the correct access rights and the repository exists.
[ERROR] 
[ERROR] Additional error details:
[ERROR] Warning: Permanently added the RSA host key for IP address '192.30.252.131' to the list of known hosts.
[ERROR] Permission denied (publickey).
[ERROR] fatal: Could not read from remote repository.
[ERROR] 
[ERROR] Please make sure you have the correct access rights
[ERROR] and the repository exists.

Unfortunately, leaflet.heat does not support bower (the maintainer has steadfastly rejected any pull requests to include a bower.json). So I explicitly added the git url. But it looks like the host resolution on Travis is unhappy with the address it's getting for github.com.

@randerzander
Copy link
Contributor

this looks awesome. maps/heatmaps are one of the most asked for features whenever I demo Zeppelin.

With this as a base, how difficult would it be to also include geojson, or have query results auto-translated into geojson map features?

@eddies
Copy link
Author

eddies commented Mar 7, 2016

@randerzander I spent a chunk of time last week playing w/ geojson support with pretty mixed results.
I probably sank the most time tracking down an annoying CSS issue that was preventing the SVG layers from rendering properly within Zeppelin. But the real difficulty for me was finding a way to pass in arbitrary geojson. I think supporting the transformation of query results into geojson features is doable, but I still think you need the ability to pass in your own geojson (e.g. a shapefile you've converted to geojson) for it to be a useful feature in Zeppelin and it was the latter that I ultimately gave up on (for the time being).

@randerzander
Copy link
Contributor

@corneadoug not sure about status of #152 , but this PR adds significant additional functionality (heatmaps). since this PR is based on @Madhuka's work, perhaps both he and @eddies can be given credit for it, if they're both agreeable to that approach?

The CI failure looks unrelated to the changes in this PR. Who can help resolve them and get these features moving?

Personally I would like to begin working on adding geojson and more geo features, but need one of either this PR, or #152 to be completed first.

@corneadoug
Copy link
Contributor

I can take a look at the CI failure.

I think the heatmap is a nice addition to the basic map. Maybe it could be toggled as an option instead of a different visualization button (just like the options in line chart)
If #152 gets done this week, I think we can rebase @eddies PR on top of it.
If #152 doesn't get done, we would go with @eddies all together.
So, If you want to get started on geojson, it could be best to base your branch on this PR

@eddies
Copy link
Author

eddies commented Mar 7, 2016

@corneadoug thank you for offering to look into the CI failure. I just successfully re-ran mvn clean package -Pspark-1.6 -Phadoop-2.6 on this branch locally. If I'm reading the travis log correctly, the CI errors are occurring because of:

ERROR org.apache.zeppelin.AbstractZeppelinIT:173 - Exception in SparkParagraphIT while testSpark
org.openqa.selenium.TimeoutException: Timed out after 60 seconds waiting for org.apache.zeppelin.AbstractZeppelinIT$1@76a123fb

@eddies
Copy link
Author

eddies commented Mar 7, 2016

I think the heatmap is a nice addition to the basic map. Maybe it could be toggled as an option instead of a different visualization button (just like the options in line chart)

I agree. In fact, what I really would want to see is an implementation of graphOptions (I think that would be the right place) that would, at a minimum, let the user select the lat/lon columns rather than the hard-coded column index assumptions.

But I figured we could call that a future enhancement request, rather than let this (or the original PR) languish any longer....

@Madhuka
Copy link
Contributor

Madhuka commented Mar 8, 2016

@corneadoug : #765 is contain the basic map feature which is in #152. #765 all checks have passed and no build failures.
@eddies : Heatmaps is nice addition for the basic map visualization, This PR ( #728 after fixing this build failure) can easily lay on top of #765

@randerzander
Copy link
Contributor

@eddies what CSS tricks did you have to do to get GeoJson to work with Leaflet in Zeppelin?

I'm trying to follow the simple example here http://leafletjs.com/examples/geojson.html . It works in a standalone webpage, but the polygons never show in Zeppelin.

Any help would be much appreciated!

@asfgit asfgit closed this in c38a0a0 May 9, 2018
asfgit pushed a commit that referenced this pull request May 9, 2018
close #83
close #86
close #125
close #133
close #139
close #146
close #193
close #203
close #246
close #262
close #264
close #273
close #291
close #299
close #320
close #347
close #389
close #413
close #423
close #543
close #560
close #658
close #670
close #728
close #765
close #777
close #782
close #783
close #812
close #822
close #841
close #843
close #878
close #884
close #918
close #989
close #1076
close #1135
close #1187
close #1231
close #1304
close #1316
close #1361
close #1385
close #1390
close #1414
close #1422
close #1425
close #1447
close #1458
close #1466
close #1485
close #1492
close #1495
close #1497
close #1536
close #1545
close #1561
close #1577
close #1600
close #1603
close #1678
close #1695
close #1739
close #1748
close #1765
close #1767
close #1776
close #1783
close #1799
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.

4 participants