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

[SQLLINE-228] Set "incremental" to false by default #229

Closed
wants to merge 1 commit into from

Conversation

liancheng
Copy link
Contributor

This PR tries to address #228 by setting the default value of option incremental to false and add an extra help entry for --incremental.

@liancheng
Copy link
Contributor Author

This PR didn't touch the docbook manual because it already declares the default value of incremental to false.

@liancheng liancheng changed the title Set "incremental" to false by default [SQLLINE-228] Set "incremental" to false by default Nov 30, 2018
@liancheng
Copy link
Contributor Author

@julianhyde Friendly ping :)

@snuyanzin
Copy link
Collaborator

From my side it looks ok. However I have a question.
@julianhyde, @liancheng do you think it would be useful to have a possibility to specify properties file to use while initialization instead of default one?
In this case it could allow to specify there incremental=false property and no other code changes in test will be required. Also it could be used as a set of default settings for tests.
I'm not suggesting do it here, just asking whether you think it would be a good refactor.

@julianhyde
Copy link
Owner

julianhyde commented Jan 7, 2019

It is a shame that there are so many changes to tests, but I think @liancheng did the right thing in preserving the current behavior of tests as much as possible.

@snuyanzin I don't think it's a good idea to have a separate properties file for init. While it would solve this problem, it would confuse users, because it's not a very useful feature for users.

I think that non-incremental mode is more important than incremental mode, so in future, I think tests should be written to in the new default, non-incremental mode. (And perhaps some of the tests that were just explicitly made incremental could be converted to non-incremental.)

Why is non-incremental mode better? Because it produces smarter column widths, and uses screen real estate more efficiently. A better user experience.

I do worry a little bit that in non-incremental mode, statements that return many thousands of rows will use a lot of memory. So, how about making such statements switch to incremental mode after N rows (say N = 1000)? (Not as part of this change; perhaps a feature request for the future?)

@snuyanzin
Copy link
Collaborator

snuyanzin commented Jan 7, 2019

Thank you for your comment. I think you are right

about

I do worry a little bit that in non-incremental mode, statements that return many thousands of rows will use a lot of memory. So, how about making such statements switch to incremental mode after N rows (say N = 1000)? (Not as part of this change; perhaps a feature request for the future?)

If am not mistaking there is a very similar feature with configured --incrementalBufferRows=NUMROWS and default 1000 in beeline https://issues.apache.org/jira/browse/HIVE-14170 which could be ported to sqlline

@julianhyde
Copy link
Owner

Thanks; I have logged #251.

@julianhyde
Copy link
Owner

Merged as 30f8ddb, fixes #228.

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.

3 participants