You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by goi <gi...@git.apache.org> on 2015/07/23 14:00:34 UTC
[GitHub] incubator-zeppelin pull request: Improve magic display system
GitHub user goi opened a pull request:
https://github.com/apache/incubator-zeppelin/pull/164
Improve magic display system
I have changed the InterpreterResult behavior so that if a magic word is detected all preceding text is omitted. This way we can use the display system and print out using the magic words in paragraphs such as spark.
example:
```
sc.parallelize(1 to 10)
```
will output:
```
res9: org.apache.spark.rdd.RDD[Int] = ParallelCollectionRDD[2] at parallelize at <console>:24
```
and mix paragraphs such as:
```
sc.parallelize(1 to 10)
println("%table col\tcol2\nqwe\t123\n")
```
will output the expected magic table
This behavior can be later extended to handle complex types.
I have also added a JUnit test for the class
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/goi/incubator-zeppelin master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-zeppelin/pull/164.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #164
----
commit f3c4df3cf8f3238f86f98c58fb9ed6e3aef84a6f
Author: cto <go...@gmail.com>
Date: 2015-07-23T11:49:07Z
improve magic word handling by omitting the prefix to the magic word
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by goi <gi...@git.apache.org>.
Github user goi commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-124088782
OK That's sounds right.
I will update the code and commit again.
Thanks,
Eran
On Thu, Jul 23, 2015 at 3:50 PM DuyHai DOAN <no...@github.com>
wrote:
> I guess that a rule saying: *if you mix several magics in the same output
> message, only the last one will be taken into account* can be a good
> start for this PR.
>
> This way, whatever the users do, they'll always have a *consistent* and
> *predictable* behavior.
>
> To implement this, maybe in the for loop, for each matching magic, you may
> save:
>
> 1. the magic word
> 2. the index of this magic word
>
> And after the for loop, just take the magic with highest index and strip
> out all the text before.
>
> How does it sound ?
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-124084610>
> .
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by elbamos <gi...@git.apache.org>.
Github user elbamos commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-128359193
Perhaps it makes sense to start moving to the point where interpret returns a collection of InterpreterResults rather than just one? Or that we allow one type of InterpreterResult to contain multiple nested InterpreterResults?
> On Aug 6, 2015, at 4:45 AM, Eran <no...@github.com> wrote:
>
> I changed the implementation according to the new rule:
> "paragraph type will be set by the first magic word in the paragraph" this allows for test such as the following to be parsed as before
>
> println(s"some text before magic word %table col1\tcol2\naaa\t%html <h3>123</h3>\n")
>
>
> also support omitting text before magic word so magic can be used in spark code
>
> val somerdd = sc.parallelize(1 to 10)
> println("%table key\tvalue\nrdd count \t%html <h3>" + somerdd.count +"</h3>\n")
>
>
> one thing to note is that it is up to the first magic word to handle the proceeding text. e.g. %table knows how to pars %html within it. but %table followed by %table will be handled differently.
>
> println("%table col A\tcol B\naaa\t123\n")
> println("%table col Z\naaa\n123\n")
>
>
> if no more changes, this is ready for merge
> Eran
>
> —
> Reply to this email directly or view it on GitHub.
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by eranwitkon <gi...@git.apache.org>.
Github user eranwitkon commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-126936773
I see, this changes the rule we thought of but it needs to be fixed.
I suggest the new rule: paragraph type will be set by the first magic word
in the paragraph. This will solve the example but in case %html some text
%table it will not process the %table.
Or do you have other suggestion for the desire outcome?
BTW, I will be oof till Wednesday so will fix it late next week.
Eran
בתאריך שבת, 1 באוג׳ 2015, 18:54 מאת Lee moon soo <no...@github.com>:
> Just before merge, i have tested one more.
>
> There were feature that display html code inside of cell in table,
> (Section 'Display as Table, chart'
> http://zeppelin.incubator.apache.org/docs/display.html)
>
> for example
>
> println(s"""%table
> key\tvalue
> a\t%html <font color=red>red</font>
> """)
>
> It supposed to be displayed in this way
> [image: image]
> <https://cloud.githubusercontent.com/assets/1540981/9022439/6c89d582-38b0-11e5-8c49-1d618b27ddc0.png>
>
> but displayed like this in this branch.
> [image: image]
> <https://cloud.githubusercontent.com/assets/1540981/9022448/adbf2548-38b0-11e5-9f21-7c808a2cfabb.png>
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-126931423>
> .
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-124600901
Perfect, it's ready to be merged then
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-124084610
I guess that a rule saying: **_if you mix several magics in the same output message, only the last one will be taken into account_** can be a good start for this PR.
This way, whatever the users do, they'll always have a **consistent** and **predictable** behavior.
To implement this, maybe in the for loop, for each matching magic, you may save:
1. the magic word
2. the index of this magic word
And after the for loop, just take the magic with highest index and strip out all the text before.
How does it sound ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by eranwitkon <gi...@git.apache.org>.
Github user eranwitkon commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-128293669
I changed the implementation according to the new rule:
"paragraph type will be set by the first magic word in the paragraph" this allows for test such as the following to be parsed as before
```
println(s"some text before magic word %table col1\tcol2\naaa\t%html <h3>123</h3>\n")
```
also support omitting text before magic word so magic can be used in spark code
```
val somerdd = sc.parallelize(1 to 10)
println("%table key\tvalue\nrdd count \t%html <h3>" + somerdd.count +"</h3>\n")
```
one thing to note is that it is up to the first magic word to handle the proceeding text. e.g. %table knows how to pars %html within it. but %table followed by %table will be handled differently.
```
println("%table col A\tcol B\naaa\t123\n")
println("%table col Z\naaa\n123\n")
```
if no more changes, this is ready for merge
Eran
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by goi <gi...@git.apache.org>.
Github user goi commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-124231907
I have updated the code to comply with this rule:
if you mix several magics in the same output message, only the last one will be taken into account
This include the following cases:
* text return text
* text before magic return magic
* magic A before magic B return magic B
* magic A before magic A' return magic A'
Updated include unit test and update to the POM
See example output:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by eranwitkon <gi...@git.apache.org>.
Github user eranwitkon commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-129221434
I hope we are going to merge this one before moving to the list
implementation. Eran
בתאריך יום א׳, 9 באוג׳ 2015, 20:36 מאת Felix Cheung <
notifications@github.com>:
> Created https://issues.apache.org/jira/browse/ZEPPELIN-212
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-129220667>
> .
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by goi <gi...@git.apache.org>.
Github user goi commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-124079421
Yes the current behavior is depending on the order of the type enums, and a better solution is to allow complex output. BUT I argue that the current implementation is not deterministic as well as the output of the magic word depends on the previous line of code and if the underling engine returned some text before.
If it is important I can change the loop to track the last occurrence of the magic word and trim all preceding text that way in your example the %html will be presented.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-126931423
Just before merge, i have tested one more.
There were feature that display html code inside of cell in table, (Section 'Display as Table, chart' http://zeppelin.incubator.apache.org/docs/display.html)
for example
```
println(s"""%table
key\tvalue
a\t%html <font color=red>red</font>
""")
```
It supposed to be displayed in this way
but displayed like this in this branch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-128523348
I guess maybe a list of InterpreterResult is where we should get to, later? so then we will have one InterpreterResult per unit of code or something.
The current approach here makes sense to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-124344667
Looks good to me. One possible refactoring to avoid the duplication of output message parsing:
```java
private TreeMap<Integer, Type> buildIndexMap(String msg) {
TreeMap<Integer, Type> typesLastIndexInMsg = new TreeMap<Integer, Type>();
if (msg.startsWith(magic + " ") || msg.startsWith(magic + "\n")) {
for (Type t : types){
lastIndexOftypes = getLastIndexOfType(msg, t);
if (lastIndexOftypes >= 0){
typesLastIndexInMsg.put(lastIndexOftypes, t);
}
}
}
return typesLastIndexInMsg;
}
```
And then re-use this method in `getType()` and `getData()`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by goi <gi...@git.apache.org>.
Github user goi commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-124600512
I have refactor the code as suggested. just one note, I dropped the line:
```
if (msg.startsWith(magic + " ") || msg.startsWith(magic + "\n")) {
```
in the suggested buildIndexMap as my understanding is that the msg can start with any text not just magic word.
All tests passed successfully.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-128861841
LGTM
Also +1 for having collection of InterpreterResult as a next step.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-zeppelin/pull/164
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-124078145
A question. What if I mix different magics ? Example:
```scala
sc.parallelize(1 to 10)
println("%table col\tcol2\nqwe\t123\n")
println("%html <h3> This is a hack </h3>")
```
In that case, what would be displayed ? A table or the HTML bloc ? I guess that in the source code of method `getType()` when we loop through all the types enums, the first type found in the output string will be retained.
But then doing `msg.substring(magicLength + msg.indexOf(magic))` will give very different results depending on whether we match **%table%** or **%html%** before.
Is this behavior deterministic or just depending on the order of the type enums ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-129220667
Created https://issues.apache.org/jira/browse/ZEPPELIN-212
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by eranwitkon <gi...@git.apache.org>.
Github user eranwitkon commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-128366820
I agree that in the long run this could be a better solution but I believe
that making small steps here is important and solve current problems.
For my users using this change is valuable by itself.
I am willing to see if I can do the next step as well once we have this one
completed.
Eran
On Thu, Aug 6, 2015 at 4:16 PM elbamos <no...@github.com> wrote:
> Perhaps it makes sense to start moving to the point where interpret
> returns a collection of InterpreterResults rather than just one? Or that we
> allow one type of InterpreterResult to contain multiple nested
> InterpreterResults?
>
> > On Aug 6, 2015, at 4:45 AM, Eran <no...@github.com> wrote:
> >
> > I changed the implementation according to the new rule:
> > "paragraph type will be set by the first magic word in the paragraph"
> this allows for test such as the following to be parsed as before
> >
> > println(s"some text before magic word %table col1\tcol2\naaa\t%html
> <h3>123</h3>\n")
> >
> >
> > also support omitting text before magic word so magic can be used in
> spark code
> >
> > val somerdd = sc.parallelize(1 to 10)
> > println("%table key\tvalue\nrdd count \t%html <h3>" + somerdd.count
> +"</h3>\n")
> >
> >
> > one thing to note is that it is up to the first magic word to handle the
> proceeding text. e.g. %table knows how to pars %html within it. but %table
> followed by %table will be handled differently.
> >
> > println("%table col A\tcol B\naaa\t123\n")
> > println("%table col Z\naaa\n123\n")
> >
> >
> > if no more changes, this is ready for merge
> > Eran
> >
> > —
> > Reply to this email directly or view it on GitHub.
> >
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-128359193>
> .
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Improve magic display system
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/164#issuecomment-126532967
Thanks for nice improvement. LGTM. While it is changing end-user experience (output handled differently in certain case), I'd like to let this open for next 24 hours and merging it if there're no more discussions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---