3
\$\begingroup\$

I have a read log file function to get select operation from database log file like this:

def getSelectMySql(log):
     with open(log,'r', encoding='utf-8', errors='ignore') as data:
        lines   = []
        for baris in data:
            bariss      = baris.rstrip()
            newBaris    = re.sub(r'\t|\n|\r|\s{2,}',' ', bariss)
            lines.append(newBaris)
            result      = []
            buffer      = []
            success     = False
            for line in lines:
                befSelect   = re.compile(r'^.+?(?=SELECT)')
                date        = re.search(r"\b(\d{6})(?=\s\d{1,}:\d{2}:\d{2})\b", line)# (\d{1,}:\d{2}:\d{2})
                select      = re.search(r'\b(SELECT)\b',line)
                parentheses = re.compile('[(){}]')
                if date:
                    dat = datetime.datetime.strptime(date.group(), '%y%m%d').strftime('%Y-%m-%d')
                    if buffer:
                        result.append(tuple(buffer))
                        buffer.clear()
                    buffer.append(dat)
                if line.endswith("important") or line.endswith("'%general_log%'") or line.endswith("puro"):
                    success = True if line.endswith("important") else False
                else:
                    if success:
                        if select and not line.endswith("SELECT"):
                            line = re.sub(befSelect,'',line)
                            line = re.sub(parentheses,'',line)
                            buffer.append(line)
            result.append(tuple(buffer))
        print('Done\n')
        return(result)

from the data, this function will one save select line after important word. the example of file, like this:

190413  7:55:31 32168376 Query  SHOW variables like '%general_log%'
        32168491 Connect    puro@17#.##.#.## on puro
        32168491 Query  SELECT * FROM `file` WHERE `identifier` = 'ca28a3b30f893899556749679f8d3066' LIMIT 1
        32168491 Quit   
        32168492 Connect    important@172.2#.#.# on important
        32168492 Query  SET NAMES 'utf8'
        32168492 Query  SHOW FULL COLUMNS FROM `sys_user`
        32168492 Query  SELECT
    kcu.constraint_name,
    kcu.column_name,
    kcu.referenced_table_name,
    kcu.referenced_column_name
FROM information_schema.referential_constraints AS rc
JOIN information_schema.key_column_usage AS kcu ON
    (
        kcu.constraint_catalog = rc.constraint_catalog OR
        (kcu.constraint_catalog IS NULL AND rc.constraint_catalog IS NULL)
    ) AND
    kcu.constraint_schema = rc.constraint_schema AND
    kcu.constraint_name = rc.constraint_name
WHERE rc.constraint_schema = database() AND kcu.table_schema = database()
AND rc.table_name = 'sys_user' AND kcu.table_name = 'sysx_user'
        32168492 Query  SELECT * FROM `lecturer_syllabus` WHERE ((`lec_id`='588') AND (`ta`='2016') AND (`sem_ta`='2')) AND (deleted !=1)
        32168492 Query  SHOW FULL COLUMNS FROM `lect_year_syllabus`

The output will be like:

[['190413', '7:55:31', SELECT * FROM `lecturer_syllabus` WHERE ((`lec_id`='588') AND (`ta`='2016') AND (`sem_ta`='2')) AND (deleted !=1)]]

But as this is my first try, I need an opinion about what I've tried, because my code runs slow with larger file.

\$\endgroup\$
3
\$\begingroup\$

Performance

Overall I believe that you are doing some things more times that you need.

  • Although compiling regexes does not take much time, if you would scan a file that has 100mln lines, the compilation on single regex (multiple times) might take up to 1 minute (it takes 108 seconds on my machine) of accumulated time. You can just compile them once before all loops (or even outside the function).

  • If I understand the code correctly, you are scanning each line many times. How about doing each line only once?

    lines = []
    for baris in data:
        # strip line and append to lines
    for line in lines:
        # do the rest
    
  • In addition to above, if parse one line at a time, it will save you a lot of memory if files are really huge (see also point 4. below).

    def getSelectMySql(log_lines_provider): # log_lines_provider is a generator that would yield one line at a time. 
        for line in log_lines_provider:
             yield extract_line_details(line)
    

Code style

Overall, I think code is not easily readable. I had to read it a few times to get a feeling of what you are trying to do.

  1. return(result) does not need brackets.
  2. I think else: if success: if select and not could be replaced by simple elif
  3. I am not sure what baris means but I think it would be more readable if you renamed this variable to line or log_line to indicate that it is one line of the log file.
  4. I would split the code to many functions e.g. :

    def getSelectMySql(log):
        lines = [] 
        with open(log,'r', encoding='utf-8', errors='ignore') as data:
            lines = get_log_lines(log) # THIS IS A BAD IDEA IF THE FILES ARE LARGE, see above
        result = []
        for line in lines: # note indent here, since you loaded all lines there is no need to keep the file open
            result.append(extract_line_details(line))
        return result
    
  5. success = True if line.endswith("important") else False can be simplified to success = line.endswith("important")

Disclaimer: all above is just my opinion, please do not treat is as a single source of truth.

\$\endgroup\$
  • \$\begingroup\$ thank you so much for your review, baris mean line (on Indonesia language). In the point 2, do you mean that I replace all of it to be just a elif or elif success == (select and not line.endswith("SELECT")) ? \$\endgroup\$ – elisa Jun 24 at 12:56
  • \$\begingroup\$ If I must split the function like in point 4, where should I put buffer list? \$\endgroup\$ – elisa Jun 24 at 13:18
1
\$\begingroup\$

To add to @MaLiN223's answer, I think adding each line to lines is unnecessary and leads to weird looping behavior. You'll wind up looping over earlier lines multiple times. For example:

lines = []

lines.append('a')
for line in lines:
    print(line)

# a

lines.append('b')
for line in lines:
    print(line)

# a
# b

I'm not sure if that's the desired behavior or not, you might need to take another look at what you're trying to accomplish. I would say it's much better to keep it as a single for loop

str.strip()

You are using excess memory and losing readability by creating new objects from baris and calling them different things. You don't use the original baris past the re.sub part, so you carry around extra objects. I would keep the name baris, as it's clear what it is: it's your looping variable:

for baris in data:
    baris = baris.rstrip() 
    baris = re.sub(r'\t|\n|\r|\s{2,}',' ', baris)

re.compile

The benefit of re.compile is you cut way down on the overhead required to re-parse your regular expression, especially in a loop:

python -m timeit -s 'import re; x = "abc123"' 'for i in range(100000): re.match("\w\d", x)'
10 loops, best of 3: 64.1 msec per loop

python -m timeit -s 'import re; x = "abc123"; y = re.compile("\w\d")' 'for i in range(100000): y.match(x)'
10 loops, best of 3: 27.8 msec per loop

However, by compiling inside a loop, you lose that benefit entirely. I would move these regular expressions:

befSelect   = re.compile(r'^.+?(?=SELECT)')
date        = re.search(r"\b(\d{6})(?=\s\d{1,}:\d{2}:\d{2})\b", line)
select      = re.search(r'\b(SELECT)\b',line)
parentheses = re.compile('[(){}]')

Up to the top, outside of your with open statement. This way you aren't re-compiling during every line, even if you don't need to.

So your regex should look like:

def getSelectMySql(log):
    befSelect_re   = re.compile(r'^.+?(?=SELECT)')
    date_re        = re.compile(r"\b(\d{6})(?=\s\d{1,}:\d{2}:\d{2})\b")
    select_re      = re.compile(r'\b(SELECT)\b')
    parentheses_re = re.compile('[(){}]')

    with open....:
        ~snip~
        for baris in data:
            ~snip~
            # compiled regex supports search, sub, find, etc.
            date = date_re.search(baris)
            select = select_re.search(baris)

            ~snip~
                baris = befSelect_re.sub('', baris)
                baris = parentheses_re.sub('', baris)

Value of success

One of the big problems I see is that you are re-setting success = False during every iteration of the file handle. I would set that outside of the for loop:

success = False
with open...

However, this brings up a new logical flow question. In your if statement:

if baris.endswith("important") or baris.endswith("'%general_log%'") or baris.endswith("puro"):
    success = True if baris.endswith("important") else False

It seems odd to check for all of them if you are going to re-check again. I'd break this into multiple statements:

if baris.endswith("important"):
    success = True
elif baris.endswith("'%general_log%'") or baris.endswith("puro"):
    success = False
else:
    # put this in one line. The ternary operator will evaluate early
    # if success is False, avoiding the rest of the boolean checks
    if success and select and not baris.endswith("SELECT"):
        ...

Overall, keeping things as a single loop will keep memory overhead down for large files, allow you to iterate over the lines once and only once, and keep your variables relatively easy to track:

def getSelectMySql(log):
    befSelect_re   = re.compile(r'^.+?(?=SELECT)')
    date_re        = re.compile(r"\b(\d{6})(?=\s\d{1,}:\d{2}:\d{2})\b")# (\d{1,}:\d{2}:\d{2})
    select_re      = re.compile(r'\b(SELECT)\b')
    parentheses_re = re.compile('[(){}]')
    spacing = re.compile(r'\t|\n|\r|\s{2,}')
    success = False

    with open(log,'r', encoding='utf-8', errors='ignore') as data:
        for baris in data:
            baris      = baris.rstrip()
            baris      = spacing.sub(' ', baris)
            result, buffer = [], []
            date        = date_re.search(baris)
            select      = select_re.search(baris)

            if date:
                dat = datetime.datetime.strptime(date.group(), '%y%m%d').strftime('%Y-%m-%d')
                if buffer:
                    result.append(tuple(buffer))
                    buffer.clear()
                buffer.append(dat)
            if baris.endswith("important"):
                success = True
            elif baris.endswith("'%general_log%'") or baris.endswith("puro"):
                success = False
            else:
                # this will terminate early if success is false
                # and won't evaluate the rest of the expression
                if success and select and not baris.endswith("SELECT"):
                    baris = befSelect_re.sub('', baris)
                    baris = parentheses_re.sub('', baris)
                    buffer.append(baris)
            result.append(tuple(buffer))
        print('Done\n')
        return result
```
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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