Proposition for pipeline system

[Andi Albrecht, 2011-09-01 11:30:42]

Just some nits :)

http://codereview.appspot.com/4965059/diff/1/sqlparse/__init__.py
File sqlparse/__init__.py (right):

http://codereview.appspot.com/4965059/diff/1/sqlparse/__init__.py#newcode58
sqlparse/__init__.py:58: from sqlparse.engine.filter import StatementFilter
Let's either add this function in the examples/ directory or replace the split()
function above with this implementation.

http://codereview.appspot.com/4965059/diff/1/sqlparse/filters.py
File sqlparse/filters.py (right):

http://codereview.appspot.com/4965059/diff/1/sqlparse/filters.py#newcode466
sqlparse/filters.py:466: def Tokens2Unicode(stream):
Where is this code used?

http://codereview.appspot.com/4965059/diff/1/sqlparse/filters.py#newcode470
sqlparse/filters.py:470: result += unicode(value)
Concatenating a string in this way isn't efficient. AFAICT the most efficient
way is simply to return ''.join(unicode(x[1]) for x in stream). Given that I
don't see a need for this function.

http://codereview.appspot.com/4965059/diff/1/sqlparse/pipeline.py
File sqlparse/pipeline.py (right):

http://codereview.appspot.com/4965059/diff/1/sqlparse/pipeline.py#newcode34
sqlparse/pipeline.py:34: if __name__ == '__main__':
The lines below should go into a unittest :)

[piranna, 2011-09-01 11:47:31]

Where i should put the comments, there or here? How i put the comments
at AppSpot?

2011/9/1  <albrecht.andi@googlemail.com>:
> Just some nits :)
>
>
> http://codereview.appspot.com/4965059/diff/1/sqlparse/__init__.py
> File sqlparse/__init__.py (right):
>
> http://codereview.appspot.com/4965059/diff/1/sqlparse/__init__.py#newcode58
> sqlparse/__init__.py:58: from sqlparse.engine.filter import
> StatementFilter
> Let's either add this function in the examples/ directory or replace the
> split() function above with this implementation.
>
> http://codereview.appspot.com/4965059/diff/1/sqlparse/filters.py
> File sqlparse/filters.py (right):
>
> http://codereview.appspot.com/4965059/diff/1/sqlparse/filters.py#newcode466
> sqlparse/filters.py:466: def Tokens2Unicode(stream):
> Where is this code used?
>
> http://codereview.appspot.com/4965059/diff/1/sqlparse/filters.py#newcode470
> sqlparse/filters.py:470: result += unicode(value)
> Concatenating a string in this way isn't efficient. AFAICT the most
> efficient way is simply to return ''.join(unicode(x[1]) for x in
> stream). Given that I don't see a need for this function.
>
> http://codereview.appspot.com/4965059/diff/1/sqlparse/pipeline.py
> File sqlparse/pipeline.py (right):
>
> http://codereview.appspot.com/4965059/diff/1/sqlparse/pipeline.py#newcode34
> sqlparse/pipeline.py:34: if __name__ == '__main__':
> The lines below should go into a unittest :)
>
> http://codereview.appspot.com/4965059/
>



-- 
"Si quieres viajar alrededor del mundo y ser invitado a hablar en un
monton de sitios diferentes, simplemente escribe un sistema operativo
Unix."
– Linus Tordvals, creador del sistema operativo Linux

[piranna, 2011-09-01 11:57:35]

http://codereview.appspot.com/4965059/diff/1/sqlparse/__init__.py
File sqlparse/__init__.py (right):

http://codereview.appspot.com/4965059/diff/1/sqlparse/__init__.py#newcode58
sqlparse/__init__.py:58: from sqlparse.engine.filter import StatementFilter
My intention was to replace split() function.

split() requires a sql string as argument, but with my conception of the
pipeline system and being able to use tokenize() as a string-to-stream filter,
it makes more sense to use split() as a stream-to-list_of_statements filter.

[piranna, 2011-09-01 12:06:27]

http://codereview.appspot.com/4965059/diff/1/sqlparse/filters.py
File sqlparse/filters.py (right):

http://codereview.appspot.com/4965059/diff/1/sqlparse/filters.py#newcode466
sqlparse/filters.py:466: def Tokens2Unicode(stream):
I'm using the function as a final step to show the result of the stream as a sql
query string. Another solution would be to add a __unicode__() and/or a
__str__() method to the stream object, but since there's no defined a class for
it (it's just a list), i didn't know what to do. One solution would be make a
dumb stream class that's just a subclass of list and have these methods defined.

http://codereview.appspot.com/4965059/diff/1/sqlparse/filters.py#newcode470
sqlparse/filters.py:470: result += unicode(value)
I like your implementation :-) It could be added to the methods i told you
before.

http://codereview.appspot.com/4965059/diff/1/sqlparse/pipeline.py
File sqlparse/pipeline.py (right):

http://codereview.appspot.com/4965059/diff/1/sqlparse/pipeline.py#newcode34
sqlparse/pipeline.py:34: if __name__ == '__main__':
On 2011/09/01 11:30:42, Andi Albrecht wrote:
> The lines below should go into a unittest :)

Totally agree, i have them here just as a quick proof-of-concept/functionality
test, but yes, this should be inside a unittest.

[Andi Albrecht, 2011-09-02 10:04:51]

With the part from __main__ moved to a unittest I'd like to pull in your changes
:)

http://codereview.appspot.com/4965059/diff/1/sqlparse/__init__.py
File sqlparse/__init__.py (right):

http://codereview.appspot.com/4965059/diff/1/sqlparse/__init__.py#newcode58
sqlparse/__init__.py:58: from sqlparse.engine.filter import StatementFilter
On 2011/09/01 11:57:35, piranna wrote:
> My intention was to replace split() function.
> 
> split() requires a sql string as argument, but with my conception of the
> pipeline system and being able to use tokenize() as a string-to-stream filter,
> it makes more sense to use split() as a stream-to-list_of_statements filter.

The result should be the same. So the only matter is which is easier to maintain
(yours) and which gives us better performance. For the latter let's leave it as
split2() for now for evaluation. At a later point we can decide again :)

[piranna, 2011-09-02 11:01:21]

> With the part from __main__ moved to a unittest I'd like to pull in your
> changes :)
>
Work in progress, i promise ;-) You'll have it at lunch or this
afternoon, i'm working at this moment... :-D


> The result should be the same. So the only matter is which is easier to
> maintain (yours) and which gives us better performance. For the latter
> let's leave it as split2() for now for evaluation. At a later point we
> can decide again :)
>
I totally agree with leave it for evaluation, i'll put the different
purpose at docstring. Respect to performance, we'll need some little
benchmarks, but at this moment, since with my pipeline allows re-use
of partial process and also works with generators for almost all, i
have got to reduce the time of my filesystem test to half :-) But
really, this would be thanks to performance or just because the re-use
of partial process (that in fact is a performance progress, but not
directly related to my pipeline generators, it would be implemented
with your system too...)


-- 
"Si quieres viajar alrededor del mundo y ser invitado a hablar en un
monton de sitios diferentes, simplemente escribe un sistema operativo
Unix."
– Linus Tordvals, creador del sistema operativo Linux

[Andi Albrecht, 2011-09-02 11:03:24]

On Fri, Sep 2, 2011 at 1:01 PM, piranna@gmail.com <piranna@gmail.com> wrote:
>> With the part from __main__ moved to a unittest I'd like to pull in your
>> changes :)
>>
> Work in progress, i promise ;-) You'll have it at lunch or this
> afternoon, i'm working at this moment... :-D

No need to hurry :)

>
>
>> The result should be the same. So the only matter is which is easier to
>> maintain (yours) and which gives us better performance. For the latter
>> let's leave it as split2() for now for evaluation. At a later point we
>> can decide again :)
>>
> I totally agree with leave it for evaluation, i'll put the different
> purpose at docstring. Respect to performance, we'll need some little
> benchmarks, but at this moment, since with my pipeline allows re-use
> of partial process and also works with generators for almost all, i
> have got to reduce the time of my filesystem test to half :-) But
> really, this would be thanks to performance or just because the re-use
> of partial process (that in fact is a performance progress, but not
> directly related to my pipeline generators, it would be implemented
> with your system too...)
>
>
> --
> "Si quieres viajar alrededor del mundo y ser invitado a hablar en un
> monton de sitios diferentes, simplemente escribe un sistema operativo
> Unix."
> – Linus Tordvals, creador del sistema operativo Linux
>