[ZEPPELIN-1486] Allow configuring whether shown values will be trimmed by mispecto · Pull Request #1458 · apache/zeppelin

mispecto · 2016-09-26T05:30:34Z

What is this PR for?

This patch improves Livy interpreter behavior: it allows configuring whether shown values will be trimmed or not.

What type of PR is it?

Improvement

Todos

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-1486

How should this be tested?

Set property zeppelin.livy.spark.sql.truncateResult to true
Run SQL query using Livy SQL interpreter
See whether long values are trimmed or not

Screenshots (if appropriate)

Questions:

Does the licenses files need update? - No
Is there breaking changes for older versions? - No
Does this needs documentation? - Yes. Documenting zeppelin.livy.spark.sql.truncateResult option.

AhyoungRyu · 2016-09-27T06:17:32Z

Hi @spektom. Thanks for your contribution :)

You said

Yes. Documenting zeppelin.livy.spark.sql.truncateResult option.

But it seems there is no docs file change. The below code needs to be added here as well.

<tr>
<td>zeppelin.livy.spark.sql.truncateResult</td>
<td>false</td>
<td>Truncate long strings in SQL display</td>
</tr>

mispecto · 2016-09-27T08:16:58Z

@AhyoungRyu Added, thanks for pointing it out to me.

r-kamath · 2016-10-04T10:07:36Z

@spektom thanks for the improvement. Can you also updateconf/zeppelin-env.sh.template and conf/zeppelin-site.xml.template with env name/property name and description.

mispecto · 2016-10-04T10:34:34Z

@r-kamath I've added documentation to the zeppelin-env.sh.template file, but I couldn't find any other option related to Livy interpreter in zeppelin-site.xml.template, so I've left this file untouched.

jongyoul · 2016-10-05T06:15:37Z

 REM set ZEPPELIN_SPARK_CONCURRENTSQL REM Execute multiple SQL concurrently if set true. false by default.
 REM set ZEPPELIN_SPARK_IMPORTIMPLICIT REM Import implicits, UDF collection, and sql if set true. true by default.
 REM set ZEPPELIN_SPARK_MAXRESULT REM Max number of Spark SQL result to display. 1000 by default.
+REM set ZEPPELIN_SPARK_TRUNCATERESULT REM Truncate long strings in SQL display. false by default.


It looks like Livy only feature for now. How about removing env here and .sh?

Agree to remove it from env.sh and site.xml, env.sh and site.xml is for zeppelin server rather than interpreter.

jongyoul · 2016-10-05T06:17:34Z

@spektom I agree with you not to add zeppelin-env.* because it depends on Live only.

zjffdu · 2016-10-08T10:03:22Z

+ "zeppelin.livy.spark.sql.truncateResult": {
+ "envName": "ZEPPELIN_LIVY_TRUNCATERESULT",
+ "propertyName": "zeppelin.livy.spark.sql.truncateResult",
+ "defaultValue": "false",


Will it make more sense to set true by default ? As it is truncated in spark by default.

zjffdu · 2016-10-08T10:06:27Z

- property.get("zeppelin.livy.spark.sql.maxResult") + ")",
+ property.get("zeppelin.livy.spark.sql.maxResult") + "," +
+ property.get("zeppelin.livy.spark.sql.truncateResult") + ")",
 interpreterContext, userSessionMap);


What error will be displayed in frontend if I set invalid value for zeppelin.livy.spark.sql.truncateResult ?

There will be an exception thrown. I don't think every property must be checked for validity in an explicit way.

What kind of exception do you see in frontend ? If user set an invalid value like 'F' and if the exception in frontend doesn't imply that, then it might be better to verify the property in method open(), otherwise it is hard to figure out what is wrong.

I don't see whether zeppelin.livy.spark.sql.maxResult or any other property is validated in any place. What's special about zeppelin.livy.spark.sql.truncateResult? Moreover, in my opinion, if user doesn't read documentation, and sets something completely wrong - it is his own problem.

@spektom I hate to be the bearer of bad news but Zeppelin is being adopted by many Enterprises and unforuantely in most cases I agree that it's the users problem. But if you are going to be committing to software that is going to be used by large enterprises then I would expect validation to occur. I am actually going to raise these issues with my Hadoop vendor.

zjffdu · 2016-10-08T10:06:59Z

+ </tr>
+ <tr>
+ <td>zeppelin.livy.spark.sql.truncateResult</td>
+ <td>false</td>


true by default ?

mispecto · 2016-10-14T15:11:46Z

Are you trying to say that people working for enterprise companies require
special input validation in addition to providing a documentation saying
that an option accepts booleans? :)
I don't think every possible input must be validated, after all these are
Software engineers who will be setting up Zeppelin...
On Fri, Oct 14, 2016 at 18:01 gss2002 notifications@github.com wrote:

@gss2002 commented on this pull request.
In
livy/src/main/java/org/apache/zeppelin/livy/LivySparkSQLInterpreter.java
#1458:
@@ -80,7 +80,8 @@ public InterpreterResult interpret(String line, InterpreterContext interpreterCo
line.replaceAll(""", "\"")
.replaceAll("\n", " ")
+ "").show(" +
 property.get("zeppelin.livy.spark.sql.maxResult") + ")",
 property.get("zeppelin.livy.spark.sql.maxResult") + "," +
 property.get("zeppelin.livy.spark.sql.truncateResult") + ")",
interpreterContext, userSessionMap);
@spektom https://github.com/spektom I hate to be the bearer of bad news
but Zeppelin is being adopted by many Enterprises and unforuantely in most
cases I agree that it's the users problem. But if you are going to be
committing to software that is going to be used by large enterprises then I
would expect validation to occur. I am actually going to raise these issues
with my Hadoop vendor.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1458, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJpO8d402flYpiA3zkU_0nYTNaePF3Pks5qz5ksgaJpZM4KGJwn
.

gss2002 · 2016-10-14T15:20:52Z

Well we will just agree to disagree

gss2002 · 2016-10-14T15:41:13Z

Also input validation is secure java coding best practice regardless.. http://www.oracle.com/technetwork/java/seccodeguide-139067.html#5

close#83close#86close#125close#133close#139close#146close#193close#203close#246close#262close#264close#273close#291close#299close#320close#347close#389close#413close#423close#543close#560close#658close#670close#728close#765close#777close#782close#783close#812close#822close#841close#843close#878close#884close#918close#989close#1076close#1135close#1187close#1231close#1304close#1316close#1361close#1385close#1390close#1414close#1422close#1425close#1447close#1458close#1466close#1485close#1492close#1495close#1497close#1536close#1545close#1561close#1577close#1600close#1603close#1678close#1695close#1739close#1748close#1765close#1767close#1776close#1783close#1799

mispecto force-pushed the ZEPPELIN-1486 branch from 589acda to e007395Compare September 26, 2016 06:14

mispecto force-pushed the ZEPPELIN-1486 branch from e007395 to 3321e18Compare September 27, 2016 08:14

mispecto force-pushed the ZEPPELIN-1486 branch from 3321e18 to 062835fCompare October 4, 2016 10:33

jongyoul reviewed Oct 5, 2016
View reviewed changes

mispecto force-pushed the ZEPPELIN-1486 branch from 062835f to e4c9abdCompare October 5, 2016 06:19

zjffdu reviewed Oct 8, 2016
View reviewed changes

mispecto force-pushed the ZEPPELIN-1486 branch from e4c9abd to 8634387Compare October 9, 2016 04:52

[ZEPPELIN-1486] Allow configuring whether shown values will be trimmed
303a8bc

mispecto force-pushed the ZEPPELIN-1486 branch from 8634387 to 303a8bcCompare October 14, 2016 06:36

asfgit closed this in c38a0a0May 9, 2018

Uh oh!

Conversation

mispecto commented Sep 26, 2016

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

AhyoungRyu commented Sep 27, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mispecto commented Sep 27, 2016

Uh oh!

r-kamath commented Oct 4, 2016

Uh oh!

mispecto commented Oct 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jongyoul commented Oct 5, 2016

Uh oh!

zjffduOct 8, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mispecto commented Oct 14, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

@gss2002 commented on this pull request.

Uh oh!

gss2002 commented Oct 14, 2016

Uh oh!

gss2002 commented Oct 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AhyoungRyu commented Sep 27, 2016•
edited
Loading

zjffduOct 8, 2016•
edited
Loading

mispecto commented Oct 14, 2016•
edited
Loading