Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

After solving the error in SO (like suggested) I return now for codereview. :-)

The task is to parse a huge file dblp.xml (~800 MB) presented by DBLP. The records in this huge file do look for example like this or this. In particular:

<?xml version="1.0" encoding="ISO-8859-1"?>
<!DOCTYPE dblp SYSTEM "dblp.dtd">
<dblp>
    record_1
    ...
    record_n
</dblp>

I wrote some code, that shall get me ech tag of some records (will bes tored in a database).

I adapted this approach from an article from IBM developerWorks which refers to the article Incremental Parsing on effbot.org. Is this the correct approach for this task? Or is there a better way?

import sys
import os
import MySQLdb
from lxml import etree


def fast_iter2(context, cursor):
    # Available elements are:   article|inproceedings|proceedings|book|incollection|phdthesis|mastersthesis|www
    elements = set(['article', 'inproceedings', 'proceedings', 'book', 'incollection', 'phdthesis', "mastersthesis", "www"])
    # Available tags are:       author|editor|title|booktitle|pages|year|address|journal|volume|number|month|url|ee|cdrom|cite|
    #                           publisher|note|crossref|isbn|series|school|chapter
    childElements = set(["title", "booktitle", "year", "journal", "ee"])

    paper = {} # represents a paper with all its tags.
    authors = []   # a list of authors who have written the paper "together".
    paperCounter = 0
    for event, element in context:
        tag = element.tag
        if tag in childElements:
            if element.text:
                paper[tag] = element.text
                # print tag, paper[tag]
        elif tag == "author":
            if element.text:
                authors.append(element.text)
                # print "AUTHOR:", authors[-1]
        elif tag in elements:
            paper["element"] = tag
            paper["mdate"] = element.get("mdate")
            paper["dblpkey"] = element.get("key")
            # print tag, element.get("mdate"), element.get("key"), event
            if paper["element"] in ['phdthesis', "mastersthesis", "www"]:
                pass # throw away "unwanted" records.
            else:
                populate_database(paper, authors, cursor)
            paperCounter += 1
            print paperCounter
            paper = {}
            authors = []
            # if paperCounter == 100:
            #     break
            element.clear()
            while element.getprevious() is not None:
                del element.getparent()[0]
    del context

def main():
        cursor = connectToDatabase()
        cursor.execute("""SET NAMES utf8""")

        context = etree.iterparse(PATH_TO_XML, dtd_validation=True, events=("start", "end"))
        fast_iter(context, cursor)

        cursor.close()


if __name__ == '__main__':
    main()
share|improve this question
    
Since you are getting an error (million author record), isn't this material for SO? –  Tshepang May 17 '11 at 12:35
    
Yes, because the code has an error it does not belong on coderview. Code review is for working code that needs improvement. –  Winston Ewert May 17 '11 at 13:58
    
I had two questions, one regarding code optimizations and one regarding the logic error. I'll try to solve the Error in SO and come back and refraze the question for code optimization. Sorry fro wrong posting. :-) –  Aufwind May 17 '11 at 14:00
add comment

1 Answer

up vote 4 down vote accepted

The name fast_iter2 seems to lack much of a connection with the function is actually doing.

Rather then having your two sets be local variables, I suggest putting them as global constants.

Calling them elements and childElements is kinda generic. I suggest something more specific

Your loop is a bit awkward. Rather then collecting them in that iterative format, just grab everything when looking at the element tags. This makes the code filling up paper/authors more straightforward.

Rather then having an empty if block and using the else block, invert the logic.

I suggest moving the element clearing code into its own function. This helps make the function actual task clearer avoiding the question of how to clear.

The del context line is useless. Del doesn't destory the object, it merely removes the object from the current context. If this was the only context in which object was refereed to then its reference count would drop to zero. However, that's not the case here as the calling function will keep it alive anyways. Even if it did, the memory savings aren't worth worrying about since the whole script is about to end anyways.

My reworking of your code, no testing has been done on it:

    CATEGORIES = set(['article', 'inproceedings', 'proceedings', 'book', 'incollection', 'phdthesis', "mastersthesis", "www"])
    SKIP_CATEGORIES = set(['phdthesis','mastersthesis', 'www'])
    DATA_ITEMS = ["title", "booktitle", "year", "journal", "ee"]

    def clear_element(element):
        element.clear()
        while element.getprevious() is not None:
            del element.getparent()[0]

    def fast_iter2(context, cursor):
        # Available elements are:   article|inproceedings|proceedings|book|incollection|phdthesis|mastersthesis|www
        # Available tags are:       author|editor|title|booktitle|pages|year|address|journal|volume|number|month|url|ee|cdrom|cite|
        #                           publisher|note|crossref|isbn|series|school|chapter

        paperCounter = 0
        for event, element in context:
            if element.tag in CATEGORIES:
                authors = [author.text for author in element.findall("author")]
                paper = {
                    'element' : element.tag,
                    'mdate' : element.get("mdate"),
                    'dblpkey' : element.get('key')
                }
                for data_item in DATA_ITEMS:
                    data = element.find(data_item)
                    if data is not None:
                        paper[data_item] = data

                if paper['element'] not in SKIP_CATEGORIES:
                    populate_database(paper, authors, cursor)


                paperCounter += 1
                print paperCounter

                clear_element(element)

EDIT

How to avoid an explicit counter:

    CATEGORIES = set(['article', 'inproceedings', 'proceedings', 'book', 'incollection', 'phdthesis', "mastersthesis", "www"])
    SKIP_CATEGORIES = set(['phdthesis','mastersthesis', 'www'])
    DATA_ITEMS = ["title", "booktitle", "year", "journal", "ee"]

    def clear_element(element):
        element.clear()
        while element.getprevious() is not None:
            del element.getparent()[0]

    def extract_paper_elements(context):
        for event, element in context:
             if element.tag in CATEGORIES:
                   yield element
                   clear_element(element)                 

    def fast_iter2(context, cursor):
        for paperCounter, element in enumerate(extract_paper_elements(context)):
                authors = [author.text for author in element.findall("author")]
                paper = {
                    'element' : element.tag,
                    'mdate' : element.get("mdate"),
                    'dblpkey' : element.get('key')
                }
                for data_item in DATA_ITEMS:
                    data = element.find(data_item)
                    if data is not None:
                        paper[data_item] = data

                if paper['element'] not in SKIP_CATEGORIES:
                    populate_database(paper, authors, cursor)


                print paperCounter
share|improve this answer
    
Thank you very much @Winston. May I ask you, how do you get to see all those things. Certainly it es a lot of experience, but there must be a way to learn faster, perhaps by books or vertain tutorials? Could you recommend something? I am eager to learn. :-) –  Aufwind May 17 '11 at 19:54
    
@Aufwind, I can't really say that its anything besides experience and a hyper-critical attitude that I'm trying to funnel in a productive manner here. I'm not aware of any tutorials that really focus on cleaning up this sort of stuff. –  Winston Ewert May 17 '11 at 20:35
1  
@Aufwind, I've posted on my blog trying to explain some of my though process to see if it helps: winstonewert.com/2011/05/reworking-code.html –  Winston Ewert May 17 '11 at 21:09
    
@Winston, the article in your blog is great. It gives me insight into your thoughts while revoking my code. Helps a lot, thank you very much! –  Aufwind May 17 '11 at 21:13
    
@Winston, you write Its possible to eliminate the explicit counting,... to these lines: paperCounter += 1; print paperCounter;. This bothers me too, but I don't know how to do it else. How would you avoid the explicit counting? –  Aufwind May 17 '11 at 21:18
show 3 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.