[ZEPPELIN-1470] limiting results from jdbc by herval · Pull Request #2428 · apache/zeppelin

herval · 2017-06-22T00:00:12Z

What is this PR for?

One thing we tracked down is that if you issue a large query on a very large table, it will simply try to load all results (and then cap them on Zeppelin's side), which seems suboptimal (and will freeze the server). Setting this on the JDBC level seems to solve the problem.

What type of PR is it?

Bug Fix

Todos

Tests

How should this be tested?

Create or use a table with a very large number of rows. In our tests, I simply created a:

createdb zeppelin_test
psql zeppelin_test
create table too_many_rows(n int)

And added 5m rows to it.

Making a paragraph like this will hang without setting a limit:

%zeppelin_test
select * from too_many_rows

Questions:

Does the licenses files need update?
No
Is there breaking changes for older versions?
No
Does this needs documentation?
No

tinkoff-dwh · 2017-06-22T17:38:23Z

user will not know that the table has more rows and the result is not complete. Maybe set fetchSize?

herval · 2017-06-22T17:47:03Z

@tinkoff-dwh you're right, picked the wrong method - using setFetchSize now

herval · 2017-06-22T17:53:19Z

I'm also not exactly sure how to write a test for this, as the JDBCInterpreterTest doesn't use anything in the line of mocks (and the outside behavior - limiting the number of returned rows - is already guaranteed by some filtering code. Any ideas @tinkoff-dwh ?

tinkoff-dwh · 2017-06-22T17:55:06Z

it would be better getMaxResult() + 1

tinkoff-dwh · 2017-06-22T18:01:48Z

I think it's impossible to test.

herval · 2017-06-22T18:02:22Z

Why +1?

tinkoff-dwh · 2017-06-22T18:12:08Z

yep, getMaxResult is correct. i thought the condition <=
while (displayRowCount < getMaxResult() && resultSet.next())
and last next() will request the next part (fetchSize)

herval · 2017-06-22T19:21:38Z

It seems apache/zeppelin master (which is what this branch is based on) is failing the build - should I rebase it to 0.7 instead, or wait for master to be fixed? Please let me know what should I do next here (this is my first contribution to the project, pardon the ignorance)

tinkoff-dwh · 2017-06-23T02:12:18Z

https://travis-ci.org/herval/zeppelin/builds/245889847
try restart 3 and 5 job

herval · 2017-06-23T21:46:01Z

@tinkoff-dwh build #3 is failing in both my master & zeppelin master: https://travis-ci.org/apache/zeppelin - restarting didn't make it pass: https://travis-ci.org/apache/zeppelin/jobs/246113686

tinkoff-dwh · 2017-06-24T10:51:44Z

failed test which load dependencies, it is possible too long or problems with the network. try restart again

zjffdu · 2017-06-27T03:18:04Z

 for (int i = 0; i < sqlArray.size(); i++) {
 String sqlToExecute = sqlArray.get(i);
 statement = connection.createStatement();
+ statement.setFetchSize(getMaxResult());


If getMaxResult() is used in statement directly, then it might not be necessary to use it in method getResults

@zjffdu
? it is count for size of fetch, not limit

Sorry, my misunderstanding of this API. Please ignore it.

tinkoff-dwh · 2017-06-27T04:42:16Z

LGTM

herval · 2017-06-28T17:54:34Z

Build's passing: https://travis-ci.org/herval/zeppelin/builds/247193502

Let me know if this is mergeable :-) @tinkoff-dwh

tinkoff-dwh · 2017-06-28T18:06:43Z

@herval
reopen pull request. and wait another reviewers or committers)

khalidhuseynov

thanks for improvement. LGTM and CI is passing, will be merging into master if no more discussion

felixcheung

should there be something to indicate the result is truncated?

herval · 2017-06-29T19:22:00Z

hmm.. It actually seems that setFetchSize does not limit the query execution (so it takes forever & hangs Zeppelin)

Figuring out if there's a way to show results are truncated + use setMaxRows

herval · 2017-06-29T19:39:48Z

tinkoff-dwh · 2017-06-29T20:00:34Z

we already have a check to limit. setMaxRows duplicate it

herval · 2017-06-29T20:11:12Z

@tinkoff-dwh it's not duplicate. The bug here is that if you don't do statement.setMaxRows, some drivers (eg postgres) will try to load everything when you issue a select *. Using setFetchSize won't affect that (I tested it). So the solution I found is to set the limit to (fetch size + 1). There's a function that will prune the results after they're back from JDBC (getResults) - I'm guessing we could remove that, if all it did was checking that limit, but don't think it's worth pursuing at this time.

TLDR - the current limit check is only applied after you get results. If you query a big enough table, you'll never get the results (and will kill Zeppelin in the process)

felixcheung · 2017-06-30T04:30:37Z

got it. I agree it's generally better to restrict the job at the source
perhaps you can use the existing settings but apply it at the JDBC instead? it would be easier to switch existing user over the new behavior this way - actually, let's see what other thinks about this approach.

herval · 2017-06-30T05:11:10Z

I am using the original setting (getMaxResults). I had to set it in two calls because setFetchSize will determine that there is a “next” page, but if I only do “setMaxResults”, it will not display the truncation warning (as it translates to a “limit”)

…

________________________________ From: Felix Cheung <notifications@github.com> Sent: Thursday, June 29, 2017 9:30:44 PM To: apache/zeppelin Cc: Herval Freire; Mention Subject: Re: [apache/zeppelin] [ZEPPELIN-1470] limiting results from jdbc (#2428) got it. I agree it's generally better to restrict the job at the source perhaps you can use the existing settings but apply it at the JDBC instead? it would be easier to switch existing user over the new behavior this way - actually, let's see what other thinks about this approach. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#2428 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAAV6lwqItW73HbpQPIjLnKd6M4nnvryks5sJHn0gaJpZM4OBqFf>.

herval · 2017-07-04T06:29:57Z

any additional thoughts on this? Is it mergeable? @khalidhuseynov @tinkoff-dwh @felixcheung

herval · 2017-07-10T16:58:20Z

Rebased to latest master - please advise

Summary: As per the PR discussion on apache#2428 Differential Revision: https://phabricator.twitter.biz/D65563

herval · 2017-07-14T03:44:04Z

ping - please advise

astroshim · 2017-07-14T07:00:55Z

LGTM~
I think this PR can be merged.

felixcheung · 2017-07-14T07:46:08Z

+
+ // fetch n+1 rows in order to indicate there's more rows available (for large selects)
+ statement.setFetchSize(getMaxResult());
+ statement.setMaxRows(getMaxResult() + 1);


so hopefully getMaxResult() won't return Integer.MAX_VALUE? because +1 will overflow?

Unless someone specifies a fetch size of two billion rows, this should be fine (the UI would break and Zeppelin would run OOM and the world would melt w/ that amount of rows anyway, so I wouldn't particularly worry about that scenario? :-))

felixcheung · 2017-07-14T17:50:30Z

merging if no more comment

### What is this PR for? One thing we tracked down is that if you issue a large query on a very large table, it will simply try to load all results (and then cap them on Zeppelin's side), which seems suboptimal (and will freeze the server). Setting this on the JDBC level seems to solve the problem. ### What type of PR is it? Bug Fix ### Todos * [x] Tests ### How should this be tested? - Create or use a table with a very large number of rows. In our tests, I simply created a: ``` createdb zeppelin_test psql zeppelin_test create table too_many_rows(n int) ``` And added 5m rows to it. Making a paragraph like this will hang without setting a limit: ``` %zeppelin_test select * from too_many_rows ``` ### Questions: * Does the licenses files need update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Author: Herval Freire <hfreire@twitter.com> Closesapache#2428 from herval/hfreire/limit-row-count and squashes the following commits: 4f66469 [Herval Freire] display truncation message b538c44 [Herval Freire] limiting results from jdbc

herval force-pushed the hfreire/limit-row-count branch from f574f84 to d3d6ebaCompare June 22, 2017 03:30

herval force-pushed the hfreire/limit-row-count branch from d3d6eba to 1061a66Compare June 22, 2017 17:46

herval force-pushed the hfreire/limit-row-count branch from 1061a66 to f751be8Compare June 26, 2017 17:49

zjffdu reviewed Jun 27, 2017
View reviewed changes

khalidhuseynov approved these changes Jun 28, 2017
View reviewed changes

felixcheung reviewed Jun 29, 2017
View reviewed changes

Herval Freire added 2 commits July 10, 2017 09:58

limiting results from jdbc
b538c44

display truncation message
4f66469

herval force-pushed the hfreire/limit-row-count branch from 4beb6d8 to 4f66469Compare July 10, 2017 16:58

dabaitu pushed a commit to dabaitu/zeppelin that referenced this pull request Jul 10, 2017
use fetch size instead of limit on jdbc
834968a
Summary: As per the PR discussion on apache#2428 Differential Revision: https://phabricator.twitter.biz/D65563

felixcheung reviewed Jul 14, 2017
View reviewed changes

felixcheung approved these changes Jul 14, 2017
View reviewed changes

asfgit closed this in 87705a9Jul 17, 2017

herval deleted the hfreire/limit-row-count branch July 17, 2017 21:43

Uh oh!

Conversation

herval commented Jun 22, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

Todos

How should this be tested?

Questions:

Uh oh!

tinkoff-dwh commented Jun 22, 2017

Uh oh!

herval commented Jun 22, 2017

Uh oh!

herval commented Jun 22, 2017

Uh oh!

tinkoff-dwh commented Jun 22, 2017

Uh oh!

tinkoff-dwh commented Jun 22, 2017

Uh oh!

herval commented Jun 22, 2017

Uh oh!

tinkoff-dwh commented Jun 22, 2017

Uh oh!

herval commented Jun 22, 2017

Uh oh!

tinkoff-dwh commented Jun 23, 2017

Uh oh!

herval commented Jun 23, 2017

Uh oh!

tinkoff-dwh commented Jun 24, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zjffduJun 27, 2017• 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!

tinkoff-dwhJun 27, 2017

Choose a reason for hiding this comment

Uh oh!

zjffduJun 27, 2017

Choose a reason for hiding this comment

Uh oh!

tinkoff-dwh commented Jun 27, 2017

Uh oh!

herval commented Jun 28, 2017

Uh oh!

tinkoff-dwh commented Jun 28, 2017

Uh oh!

khalidhuseynov left a comment

Choose a reason for hiding this comment

Uh oh!

felixcheung left a comment

Choose a reason for hiding this comment

Uh oh!

herval commented Jun 29, 2017

Uh oh!

herval commented Jun 29, 2017

Uh oh!

tinkoff-dwh commented Jun 29, 2017

Uh oh!

herval commented Jun 29, 2017

Uh oh!

felixcheung commented Jun 30, 2017

Uh oh!

herval commented Jun 30, 2017 via email

Uh oh!

herval commented Jul 4, 2017

Uh oh!

herval commented Jul 10, 2017

Uh oh!

herval commented Jul 14, 2017

Uh oh!

astroshim commented Jul 14, 2017

Uh oh!

felixcheungJul 14, 2017

Choose a reason for hiding this comment

Uh oh!

hervalJul 14, 2017

Choose a reason for hiding this comment

herval commented Jun 22, 2017•
edited
Loading

tinkoff-dwh commented Jun 24, 2017•
edited
Loading

zjffduJun 27, 2017•
edited
Loading