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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better default options and configuration for the CSV import #1395

Closed
MKleusberg opened this Issue May 22, 2018 · 6 comments

Comments

Projects
None yet
2 participants
@MKleusberg
Copy link
Member

MKleusberg commented May 22, 2018

@karim came up with a good default behaviour for the CSV import in #1349:

I just want to say something. 馃槂

Regarding the expected behavior of DB4S when importing empty values from a CSV.

  1. If the column has a default value then it gets priority over everything else.
    • This is the default action for SQLite, it fills columns with default values when a value is not given.
    • The column being NOT NULL or not doesn't matter, since a value will be set for it anyway.
    • 鉁旓笍 No need for a checkbox, it should be the right thing to do. 馃憤
  2. If the column does not have a default value.
    1. Check if the column does not have NOT NULL, if true:
      • Then just add NULL to it, it should be the default.
      • Be consistent. CSV value is NULL, SQLite table doesn't forbid NULL, so just add NULL.
      • 鉁旓笍 Also, no need for a checkbox, and this is what everyone here wants. 馃帀
    2. Else if it does have NOT NULL, then we have two options, either:
      1. Fail the import (make sense since you are importing NULL to a NOT NULL column), or
      2. Set a default value for it (e.g. 0 for INTEGER, "" for TEXT)
        • Probably by adding a checkbox to "set default values for empty fields in NOT NULL columns" (need a better wording 馃槃)

We should implement some rules in the import code to reflect this and maybe add some configuration options to make any deviation from this default an explicit decision by the user.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented May 22, 2018

Oh, we should probably also have an option to turn off the data type autodetection. Or at least have the values given in quotes "foo" always be strings.

I personally find the autodetection useful, but it was pointed out in #1382 that for some use cases it's a bad thing and leads to data loss.

Personally, I'm really bothered by our lack of warning to users when we mis-import stuff too, but I have no solid idea how to fix it just yet.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented May 22, 2018

... and there's #1347 as well. That one looks reasonably simple though. 馃槃

MKleusberg added a commit that referenced this issue May 25, 2018

Implement better default behaviour for CSV import
This changes the default behaviour for the CSV import to follow a set of
rules which hopefully makes most people happy.

It also add an "Advanced" section to the settings bits of the dialog to
modify this new default behaviour.

See issue #1395.
@MKleusberg

This comment has been minimized.

Copy link
Member Author

MKleusberg commented May 25, 2018

Just pushed a first attempt at implementing @karim's set of rules above. We're not failing the import by default as suggested in 2.II.a, but there is a new option to make it fail in that case.

A few more notes and questions:

  1. This is more or less untested. Please give it a try and let me know if it works for you 馃槈
  2. In the process we have lost one option: deciding whether to insert a NULL or an empty string if both would work for the table layout (most notably when importing into a new table). We now always insert NULL in that case. Does anybody miss the freedom to make it import an empty string here?
  3. Because the two new configuration options are by default hidden, I'm not saving and loading the last used settings. What do you think here? Maybe save them anyway (but then you don't see it's not the default behaviour)? Or save them and if they differ from the defaults, show the Advanced section too (but then it's not really "advanced" anymore)?
@MKleusberg

This comment has been minimized.

Copy link
Member Author

MKleusberg commented Jun 7, 2018

If nobody has reservations about my decisions regarding question 2 and 3 in my last comment, I would close this issue because the other issues mentioned by @justinclift already have their own tickets 馃槃

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Jun 7, 2018

No reservations from me.

I reckon we should tweak the names and tooltip descriptions a bit for the CSV options, but that's easily a follow up thing for later. 馃槃

MKleusberg added a commit that referenced this issue Jun 7, 2018

Implement better default behaviour for CSV import
This changes the default behaviour for the CSV import to follow a set of
rules which hopefully makes most people happy.

It also add an "Advanced" section to the settings bits of the dialog to
modify this new default behaviour.

See issue #1395.
@MKleusberg

This comment has been minimized.

Copy link
Member Author

MKleusberg commented Jun 7, 2018

Ok cool, I'll close this issue then 馃槃

@MKleusberg MKleusberg closed this Jun 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment