Review Request: DB: change lyrics table: text url -> integer url pointing to the urls table

[Matěj Laitl]

> On May 21, 2012, 8:54 a.m., Bart Cerneels wrote:
> > ChangeLog, line 112
> > <http://git.reviewboard.kde.org/r/104966/diff/2/?file=64618#file64618line112>
> >
> >     Does this patch fix this? Or just makes it so rescanning is not required anymore?

Makes rescanning not needed anymore, as mentioned in the commit msg.


> On May 21, 2012, 8:54 a.m., Bart Cerneels wrote:
> > src/core-impl/collections/db/sql/DatabaseUpdater.cpp, line 642
> > <http://git.reviewboard.kde.org/r/104966/diff/2/?file=64620#file64620line642>
> >
> >     You don't actually check if the previous query succeeded.

My bad, leftover from the previous versoin of the patch.


> On May 21, 2012, 8:54 a.m., Bart Cerneels wrote:
> > ChangeLog, line 50
> > <http://git.reviewboard.kde.org/r/104966/diff/2/?file=64618#file64618line50>
> >
> >     "data are migrated" tickles my English grammar bone. "data is migrated" sounds better, even though it might not technically be correct with data plural.

Fixed, thanks.


> On May 21, 2012, 8:54 a.m., Bart Cerneels wrote:
> > src/core-impl/collections/db/sql/DatabaseUpdater.cpp, line 865
> > <http://git.reviewboard.kde.org/r/104966/diff/2/?file=64620#file64620line865>
> >
> >     I was wondering if the url column could actually be intentional to point to a remote resource. But if it's not used currently there is no point in having it.

It is only used from the SqlTrack, plus only the rpath part of it, e.g. it is even not the full url, but only the part after the mountpoint.


- Matěj


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104966/#review14013
-----------------------------------------------------------


On May 22, 2012, 8:04 p.m., Matěj Laitl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104966/
> -----------------------------------------------------------
> 
> (Updated May 22, 2012, 8:04 p.m.)
> 
> 
> Review request for Amarok and Ralf Engels.
> 
> 
> Description
> -------
> 
> DB: change lyrics table: text url -> integer url pointing to the urls table
> 
> I believe that the old lyrics table structure was more or less a mistake:
> TABLE lyrics (
>     id INTEGER PRIMARY KEY, -- actually never used in code
>     url VARBINARY(324), -- actually a rpath from urls table
>     lyrics TEXT
> )
> 
> Apart from data duplication and non-conformity to the "update anomaly"
> requirement of the database design, there was additional problem that rpath
> itself is not unique. The (deviceId, rpath) is.
> 
> This change makes the lyrics table look more sane:
> TABLE lyrics (
>     url INTEGER PRIMARY KEY, -- points to url.id column
>     lyrics TEXT
> )
> 
> with an effort to transition existing entries. The transition of 5000
> lyrics entries takes 16s on my 2.5 GHz Core i5 (one core used), so it
> should be acceptable.
> 
> This is the first time I'm changing db structure, I'd be glad to
> receive thorough review, namely of the update13to14() method and
> especially the duplicate-removing query. This is critical because
> database-corrupting fault would leave many Amarok users in a state
> where they would need to drop their database to make Amarok working
> again.
> 
> Note to reporters of bug 242350: there's an unrelated bug 299150 which
> now applies to lyrics, too.
> 
> ChangeLog of the unrelated iPod fix is updated because DB_VERSION bump
> triggers full collection rescan as far as it is documented.
> 
> BUG: 242350
> FIXED-IN: 2.6
> REVIEW: 104966
> 
> 
> This addresses bug 242350.
>     https://bugs.kde.org/show_bug.cgi?id=242350
> 
> 
> Diffs
> -----
> 
>   ChangeLog 67bc020387a1b4bc8988c2688a82976eb49fed2f 
>   HACKING/mysql_database_schema.txt 547ffd5385b82523ced1038606db154b5cd3f640 
>   src/core-impl/collections/db/sql/DatabaseUpdater.h 37ccb54197a8b63c77b3bb7bcceaae838db56538 
>   src/core-impl/collections/db/sql/DatabaseUpdater.cpp e8cbd42de15ee976de37eeff866d4596c3173328 
>   src/core-impl/collections/db/sql/SqlMeta.cpp f8f9bdb7f0c83d3584ebfe2ce1c044bc54495885 
> 
> Diff: http://git.reviewboard.kde.org/r/104966/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120522/42aed3c4/attachment-0001.html>