I've added some more comments to GitHub. The short summary is this all looks good to me.
The main concern I have is with the handling of aliases. However, I don't think that's something you can fix here; rather, I think there probably needs to be a bit more thought about this in afw::table. I have no problem with the way you've tackled this being merged, but it's probably worth opening a ticket against afw to revisit this issue.
I particularly like the test case, which is nice and comprehensive. However... it's unfortunate that it requires talking to a live MySQL instance to run. It seems like you could carry out some quite useful testing in the absence of that by doing some mocking – you know a priori what the stream of bytes you want to send to the database is, so you should be able to use a mock database connection to demonstrate that that really is what gets generated by your task. I'd imagine that setting up the infrastructure for that might take some thought, so I'm not suggesting it should be a requirement for getting this ticket merged, but I wonder if this is something we ought to consider more generally for testing code which interacts with databases.
In this case, you could also write more specific unit tests rather than just demonstrating that the whole task works. For example, I think I'd have found a test case which just exercises aliasesFor useful when understanding what the code does (and I'd imagine it would have been useful when writing the code). Again, though, I don't think this is essential for completing the ticket – I'm happy that the existing test case does a good job of covering all the functionality.
A couple of final notes which don't directly related to the code:
- What's the future of the datarel package? It seems (from reading the "related issues" links) that it should be removed after you've extracted all the still-relevant code from it (which you've done here), but that dropping it entirely is a problem for buildbot (
DM-2948). After this ticket is complete, is the only blocker to removing that package the buildbot issue?
- There are some pages on Confluence (Tour of the software stack, Package & task index) which specifically refer to datarel. Please could you update them to refer to daf_ingest instead/as well (as appropriate)? (They also mention ap – I guess that can be dropped?)