Reading a gzipped file with JSON data for thousands of users

Question

I'm a newbie in Python.

I need to read a GZ file contains thousands of users. But I think my code is not optimized in this case. Could anyone can help me? What should I do to improve performance? Or is it standard?

if os.path.isfile("file.gz"):
    with gzip.GzipFile("file.gz", 'r') as fin:   
        for line in fin:
            if get_new_user is True:
                if datetime.strptime(json.loads(line).get('UpdatedAt'), '%Y-%m-%dT%H:%M:%S.%fZ').date() == (datetime.today()-timedelta(1)).date():
                    users.append(json.loads(line))
            else:
                users.append(json.loads(line))    
    os.remove("file.gz")
    return users

Answer 1

It looks like your code is somewhat non-optimal.

1. In the case get_new_user is True, you call json.loads(line) once for the "UpdatedAt" test, and if that passes, you call it again, returning effectively the same value, to append the users list.
2. In the case of get_new_user is True, you call (datetime.today()-timedelta(1)).date() once for each user, yet it is effectively constant. You should move this out of the loop.

You could refactor your code like this:

threshold = (datetime.today()-timedelta(1)).date()
for line in fin:
    user = json.loads(line)
    if get_new_user is True:
        if datetime.strptime(user.get('UpdatedAt'), '%Y-%m-%dT%H:%M:%S.%fZ').date() == threshold:
            users.append(user)
    else:
        users.append(user)    

You are doing the get_new_user is True test for each user/line in the file. That too is constant, and only needs to be done once. It is a tiny gain, but you could put the for loop inside the if and else clauses.

if get_new_user is True:
    threshold = (datetime.today()-timedelta(1)).date()
    for line in fin:
        user = json.loads(line)
        if datetime.strptime(user.get('UpdatedAt'), '%Y-%m-%dT%H:%M:%S.%fZ').date() == threshold:
            users.append(user)
else:
    for line in fin:
        users.append(json.loads(line))

And maybe using some list comprehensions and a generator expression for fun ...

if get_new_user is True:
    threshold = (datetime.today()-timedelta(1)).date()
    users = [ user for user in (json.loads(line) for line in fin)
              if datetime.strptime(user.get('UpdatedAt'),
                                   '%Y-%m-%dT%H:%M:%S.%fZ'
                                  ).date() == threshold ]
else:
    users = [ json.loads(line) for line in fin ]

Or if you are truly brave ... almost a 1-liner:

threshold = (datetime.today()-timedelta(1)).date()
users = [ user for user in (json.loads(line) for line in fin)
          if get_new_user is not True or
             datetime.strptime(user.get('UpdatedAt'),
                               '%Y-%m-%dT%H:%M:%S.%fZ'
                              ).date() == threshold ]

---

I've left your if get_new_user is True: statement alone, but it is a little non-Pythonic. Usually, you'd just use if get_new_user:, but those two statements are very different. The first is true if and only if get_new_user is True whereas the second would be true when get_new_user contains any non-falsey value, such as: True, or "True" or 1, or 17 or "False", or [0], ... etc.

Answer 2

Let's break-down your existing code:

if os.path.isfile("file.gz"):
    with gzip.GzipFile("file.gz", 'r') as fin:   
        for line in fin:

The above code would work faster if you made this into a separate function and loaded the file fully first (perform all the I/O in a single operation, loading the data into the memory (much faster), rather than keeping the loop as incrementing the file pointer to read the next line). I/O is slow, always try to get files into a memory structure before doing anything.

            if get_new_user is True:

This line would be evaluated every single time the loop runs. We should avoid that.

                if datetime.strptime(json.loads(line).get('UpdatedAt'), '%Y-%m-%dT%H:%M:%S.%fZ').date() == (datetime.today()-timedelta(1)).date():

This line executes datetime.today() every single time the loop hits this line. Again, something to avoid by placing the operation result into a variable, before the loop runs.

                    users.append(json.loads(line))
            else:
                users.append(json.loads(line))    

This is the same line - both sides of the if statement run this exact statement regardless of the result of the date check - meaning the date check is kind of useless - AND the load operation is duplicated/can be separated, right? Perhaps you are missing a line of code? perhaps this is a logic error? It would be helpful to get a comment regarding this.

    os.remove("file.gz")
    return users

you miss the initial declaration of the users data type. This statement will return either None or (I'm guessing) a list (because you use .append()).

With these comments, you should be able to make a piece of code that does something like (this is pseudocode):

function return_file_data(filename)
  if file_exists(filename)
   data = filename.read_all() #including gunzip
   remove(filename)

  return data or None

function process_data(data)
  users = []
  for line in data
    users.append(json.loads(line))

  return users

function main(filename)
  data = return_file_data(filename)
  if data
    users = process_data(data)
    # do something with the users data

if __name__ == "__main__":
  filename = get_filename() # I'm sure you do that somewhere?
  main(filename)

You'll notice I dropped the datetime comparison AND the get_new_user flag comparison - for reasons mentioned earlier. If you improve your code (as per codereview guidelines), create a new post (with your new code) and reference this post for historical purposes.

Hope this helps!