Repository navigation
Batch import multiple CSV files at once - #1018
Conversation
- Tweak input checker - Preserve old file import as not to cause any unforeseen breaks - Allow ignoring file name when importing multiple files to tables - Mass toggle several files for import
|
Oh wow. Animated gif showing the new behaviour in the PR. Now that's got to be a gold standard for PR things-to-do. 😁 |
|
Impressive! 😄 Few questions:
Not questioning your work, it looks very nice. But we need to document this behavior. Thanks for your hard work 👍 |
|
@karim This PR updates |
|
Ahhh. Good to know. Thanks @justinclift 😄 |
Depends on the OS and version. Some older versions of Windows have a hard coded limit of 200 files. |
The way this feature worked initially and still does with this multiple selection variation is that it fetches the list of names first (that a user can then cherry pick from) then opens a stream, parses and performs actions after in a sequential order over the selected files. The open dialog filters what formats are allowed and the internal parser will warn/skip/err on bad files (invalid csvs). I had a folder with ~7k files with thousands of rows and including decoding and inserting took a little over half hour on a Dell XPS 15 running x64 Win 10.
I added an (experimental until review) feature to Match similar files by checking headers (the green highlight in the gif above) so once you have selected a list of files there is no need to manually sort them before to ensure they are in the same format, hence no need to prompt for more files. @chrisjlocke Anything below WinXP was hardcoded due to memory constraints and dependent on runtimes. IIRC WinXP itself had a limit of |
MKleusberg
left a comment
There was a problem hiding this comment.
Thanks a lot for this, @iKlsR 👍 I'll need some time to review this but my first impression is very good. I hope I'll finish the review over the weekend 😄
MKleusberg
left a comment
There was a problem hiding this comment.
One minor thing I've noticed so far: in the Import CSV dialog you could update the tab order because right now the newly added items come after the preview table view and the Separate tables checkbox comes after the file list.
|
@MKleusberg Ah! Thanks, forgot about that and looking forward to it. |
| } | ||
| } | ||
|
|
||
| sqlb::FieldVector ImportCsvDialog::fetchCsvHeader(const QString& fileName) |
There was a problem hiding this comment.
This function is more or less a copy of importCsv(). I think we should modify it slightly, so it can be called from inside importCsv() as well to share the common code. Maybe change it to take a pointer to a CSVParser object and use that instead of taking a file name and opening the file etc. Though I'm not exactly sure how to do that ideally...
There was a problem hiding this comment.
Though I'm not exactly sure how to do that ideally...
Had the same thought then that :), it was a bit tricky seeing how the FieldVecs and csv object are used because I didn't want to upset or refactor too much code initially but I'll come up with something.
|
|
||
| QTextStream tstream(&file); | ||
| tstream.setCodec(currentEncoding().toUtf8()); | ||
| csv.parse(tstream); |
There was a problem hiding this comment.
When fetching just the headers, you should be able to pass 1 as the second argument to parse(), like this:
csv.parse(tstream, 1);This limits the number of records to just the first one which should be enough for the header. This way you don't have to parse the entire file and (if this code isn't shared with importCsv()) you can get rid of the progress dialog entirely to simplify the code a bit and prevent any flickering from quickly opening and closing windows.
There was a problem hiding this comment.
Ah, the holy grail, I knew I forgot something and hoped there would be a way to just get the header (didn't look hard enough really). Thanks for the clarification.
|
Hi Ricardo! Sorry for the delay! I've finally got around to review your entire PR 😄 It looks very, very well done. Besides the tab order thing I have only added two more comments but these are somehow related. They are both about the One extra thought I had is that it would be nice to specify table names in 'Separate tables' mode. Right now a new table is created for each file but ideally you would be able to set a table name for each file and default it to the file name. This could be done by making the table name field editable and let it show the table name for the currently selected table, or by adding an extra column to the file list view. But that's just an idea. Your PR is definitely already a huge improvement the way it is 😄 If you ever feel like implementing this idea it can always be done in a second PR. |
|
Thanks! 😊 Perfect timing as well, can add it to my free time session tomorrow. |
|
Awesome 😄 Looking forward to the update. |
|
Changes will be up in a few™, was a bit busy. |
|
@MKleusberg Done, think I found the cleanest way to delegate and reuse the csv and field generation, fixed tab order and incorporated your suggestion to make each table have a unique name. Also added a auto toggle to not show progress dialog when matching similar files. Overall, the dialog feels a lot smoother and snappy. |
|
Awesome! 👍 This is looking really good now. I think we can mention this feature (maybe including you gif) in the Highlights section of the next release notes. I'll keep it in 7 commits instead of squashing them into one, too, because they are nice, separate units. Thanks again for your efforts here 😉 Oh, and even though it's a little late already: welcome to the team, @iKlsR! 😄 |
Allow for batch importing multiple CSV files at once using the import CSV dialog
Fixes #1002
Code could be a lot slimmer, didn't poke around as much as I could, "first timer" so a bit of reuse and ideally we should be able to fetch headers without running through the entire file (so the last commit was more of a poc) but would make for a more streamlined experience. Suggestions and improvements welcome ofc.