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

Save current filter, sort column and display formats as a new view #1246

Merged
merged 3 commits into from Dec 1, 2017

Conversation

Projects
None yet
3 participants
@mgrojo
Copy link
Contributor

mgrojo commented Nov 30, 2017

A new button is added to the Browse Data tab for saving the current display
of the table (current filter, sort column and display formats) as a new
view. This allows (specially for non advanced users) the creation of simple
views. It can be seen, either as a way of storing the current
filtering or as an easy way of creating views.

This reuses the query set in sqlitetablemodel, but the column aliases, when
a display format is used, has been changed from col%1 to the original column
names, i.e.: format(orig) AS orig.

That is the part where I am not sure about these changes. Can this name preserving break anything? Apparently not, but I don't dare to commit this without review.

This change can be made compatible with the other pull request that I opened, but they overlap somehow and this concept is more powerful when one tries to save a filter for later usage. if this is merged, maybe the other is not worth enough for merging.

Save current filter, sort column and display formats as a new view
A new button is added to the Browse Data tab for saving the current display
of the table (current filter, sort column and display formats) as a new
view. This allows (specially for non advanced users) the creation of simple
views. It can be seen, either as a way of storing the current
filtering or as an easy way of creating views.

This reuses the query set in sqlitetablemodel, but the column aliases when
a display format is used has been changed from col%1 to the original column
names, i.e. format(`orig`) AS `orig`.
@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Nov 30, 2017

Cool @mgrojo this looks useful for people. Haven't tried it yet (am getting backups working for other bits), but probably will over the next few days if it's not already merged. 😄

Remove no longer used method SqlExecutionArea::saveAsView()
It was moved to MainWindow for reuse and this copy was no longer used.
@MKleusberg
Copy link
Member

MKleusberg left a comment

This is definitely a good idea 😄 There is one problem though which will break filtering tables with a custom display format, so that should be addressed first. But other than that I have only found some minor things 😄

while(true)
{
name = QInputDialog::getText(this, qApp->applicationName(), tr("Please specify the view name")).trimmed();
if(name.isEmpty())

This comment has been minimized.

@MKleusberg

MKleusberg Dec 1, 2017

Member

One minor thing here: technically this should be .isNull() instead of .isEmpty(). This makes it possible to create views with an empty name. It was .isEmpty() in the old code and we're not really consistent here but I thought it might make sense to change this while working on this code anyway 😄

saveAsView(m_browseTableModel->customQuery(false));
else
QMessageBox::information(this, qApp->applicationName(), tr("There is not any filter set for this table. View will not be created."));

This comment has been minimized.

@MKleusberg

MKleusberg Dec 1, 2017

Member

Extra empty line here 😉

// Save as view a custom query without rowid
saveAsView(m_browseTableModel->customQuery(false));
else
QMessageBox::information(this, qApp->applicationName(), tr("There is not any filter set for this table. View will not be created."));

This comment has been minimized.

@MKleusberg

MKleusberg Dec 1, 2017

Member

The wording seems a bit odd here. Maybe change it to "There is no filter set for this table".

@justinclift What do you think as a native speaker? 😉

This comment has been minimized.

@mgrojo

mgrojo Dec 1, 2017

Author Contributor

Ok, your proposal sounds better for me too 😄 I'll change that but I'd like to hear Justin's opinion.

This comment has been minimized.

@justinclift

justinclift Dec 1, 2017

Member

"There is no filter set for this table" sounds fine to myself as a native English speaker. Someone really into correct English sentence structure might have a problem with it (not sure), but to me it sounds perfectly ok from a "natural" point of view. 😄

column = m_headers.at(i.key());
where.append(QString("%1 %2 AND ").arg(sqlb::escapeIdentifier(column)).arg(i.value()));
QString column = sqlb::escapeIdentifier(m_headers.at(i.key()));
where.append(QString("%1 %2 AND ").arg(column).arg(i.value()));

This comment has been minimized.

@MKleusberg

MKleusberg Dec 1, 2017

Member

This won't work as expected. The filters will then be applied to the original data, not the converted data. E.g. using the Julian day format leads to this SQL:

SELECT datetime(`my_date_column`) AS `my_date_column` WHERE `my_date_column` LIKE '%2017%';

vs this with the old code:

SELECT datetime(`my_date_column`) AS `col1` WHERE `col1` LIKE '%2017%';

In SQLite the results will be different because in the first case the WHERE part is applied first and the datetime function will only be applied on the resulting data set afterwards.

This comment has been minimized.

@mgrojo

mgrojo Dec 1, 2017

Author Contributor

Ok, there's no easy way 😢 I can fall back to my first idea that was not saving the display formats, because I don't know how can it be made to work preserving the original column names. I don't like either adding a prefix like new_ or just _ to all the columns.

Fixed issues from @MKleusber's review
New view name validated with isNull().

Text in information box reworded.

Column names are preserved in the new view, except for the ones with
display formats, for which an underline is appended in order to fix the
reported filtering defect.
where.append(QString("%1 %2 AND ").arg(column).arg(i.value()));
QString columnId = sqlb::escapeIdentifier(m_headers.at(i.key()));
if(m_vDisplayFormat.size() && m_vDisplayFormat.at(i.key()-1) != columnId)
columnId = sqlb::escapeIdentifier(m_headers.at(i.key()) + "_");

This comment has been minimized.

@MKleusberg

MKleusberg Dec 1, 2017

Member

But that breaks for tables like these:

CREATE TABLE `test` (
	`field`	INTEGER,
	`field_`	INTEGER
);

On the other hand, the same was true for the old code, just with different column names. So I think we can leave it like this unless you have an idea how to fix this easily 😄

This comment has been minimized.

@mgrojo

mgrojo Dec 1, 2017

Author Contributor

Well, in fact I started adding "_" to all columns and that is the safe approach. But it was so ugly 😄 I think, I'll go back to that. Better ugly than having broken combinations.

This comment has been minimized.

@mgrojo

mgrojo Dec 1, 2017

Author Contributor

I'm surprised. sqlite is still filtering by the wrong column when all fields have been renamed. For your example table:
SELECT _rowid_,upper(field) AS field_,field_ AS field__ FROM main.test WHERE field_ = 'hola' ORDER BY _rowid_ ASC LIMIT 0, 50000;
That filters by field__ (original field_) and not by the new field_. If sqlite is going to always give precedence to the original columns, I don't see a efficient and simple way to solve the problem. As you said, the original implementation had the same problem when there were original columns named col1, col2, etc.

So I'll merge from my last commit 😄

This comment has been minimized.

@MKleusberg

MKleusberg Dec 2, 2017

Member

Yeah, I think the only way to do this properly is to apply the display format conversion to the column and to the WHERE clause:

SELECT DATETIME(field) AS field, field_ FROM test WHERE DATETIME(field) LIKE '%2017%';

This comment has been minimized.

@mgrojo

mgrojo Dec 2, 2017

Author Contributor

Yes, that is the standard and safe solution. What I have read is that any WHERE applied to the aliases is actually disallowed by the SQL standard. I've implemented your idea and works well. Do you think the display format would be calculated twice or it would be optimized? In any case I think it's the best solution.

https://stackoverflow.com/questions/8370114/referring-to-a-column-alias-in-a-where-clause
https://dev.mysql.com/doc/refman/5.5/en/problems-with-alias.html

This comment has been minimized.

@MKleusberg

MKleusberg Dec 2, 2017

Member

Looks like it is calculated twice. But whatever, implementing this correctly seems more important to me than the slight performance benefit 😄

CREATE TABLE `test` (
	`field`	INTEGER,
	`field_`	INTEGER
);
EXPLAIN SELECT DATETIME(field) AS field, field_ FROM test WHERE DATETIME(field) LIKE '%2017%';

prints

addr opcode p1 p2 p3 p4 p5 comment
0 Init 0 13 0 00
1 OpenRead 0 2121 0 2 00
2 Rewind 0 12 0 00
3 Column 0 0 4 00
4 Function0 0 4 3 datetime(-1) 01
5 Function0 1 2 1 like(2) 02
6 IfNot 1 11 1 00
7 Column 0 0 1 00
8 Function0 0 1 5 datetime(-1) 01
9 Column 0 1 6 00
10 ResultRow 5 2 0 00
11 Next 0 3 0 01
12 Halt 0 0 0 00
13 Transaction 0 0 416 0 01
14 String8 0 2 0 %2017% 00
15 Goto 0 1 0 00

@mgrojo mgrojo merged commit 882fc8d into master Dec 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mgrojo mgrojo deleted the save_filter_as_view branch Dec 1, 2017

mgrojo added a commit that referenced this pull request Dec 2, 2017

Improvements in the new "Save filter as view" functionality
The query used in sqlitetablemodel does not rename the columns, instead
it applies the display formats in the WHERE part. In this way the saved
view has the original column names.

Changes in Button "Save filter as view": more information in tooltip and it
is disabled when no database is open.

See discussion in PR #1246
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment