[storm] Oracle support review

[Gustavo Niemeyer]

Gustavo,

I've just had a look at your branch implementing Oracle support for
Storm, and it actually surprised me, since it feels like it's pretty close
to being mergeable.

It'd be awesome if more people who have hands on access to
an Oracle database could give it a try too.

I provide an initial review below, in the numbered style we're used to
so that it's easy to refer to specific parallel discussions.

[1]

+    if to_param_mark == ':1':
+        cnt = 1
+        for i in range(0, len(tokens), 2):

I'm not 100% sure of what's going on here, but if the idea is replacing the
"?" marks with consecutive ":1", ":2" , etc, I suggest using something like
this instead of parsing it manually:

>>> from_mark_pattern = re.compile(r"\?")
>>> next_to_id = iter(xrange(1, 100000)).next
>>> def next_to_mark(match):
...     return ":%d" % next_to_id()
...
>>> from_mark_pattern.subn(next_to_mark, "SELECT ?, ?, ?")
('SELECT :1, :2, :3', 3)

[2]

+    compile_sql_token)
+
+install_exceptions(oracle)

That's a minor issue, but we try to follow most of the style recommendations
in Python's PEP-8.  So two lines of spacing between imports and body, two
lines between classes or top-level functions, one line between methods. Also
imports are ordered like stdlib first, then third party, then local modules.

Don't worry too much about this though.  We can even fix it up for you
before integration if you want.

[3]

+    # this is a simple hack to make sure all auto-generated aliases
+    # get escaped; I added this if because oracle was complaining of
+    # stuff such as money."value", that was being generated;

Do you know what exactly is the syntax that Oracle complains about?
We can probably try to fix it without any hacks.

[4]

+        super(OracleResult, self).__init__(connection, raw_cursor)
+        self.lastrowid = rowid

This should be private, since it's not part of the API offered by Storm itself.
At least not yet.  We've been pondering about adding a standard way to
access such entries.

It might also be good to look at the mysql-insert-id branch from James
which is up for review.  It has some optimizations to avoid one extra
query on inserts which perhaps could be used here too.

[5]
+    def execute(self, statement, params=None, noresult=False):
+        """NOTE: this is being overriden completely because the
+        original from the base class expects to receive only a
+        raw_cursor from raw_execute, and we need to receive also the
+        rowid, as we cannot set it in the cursor object

I don't understand all the details here, but this should be avoided if
possible, since it's duplicating a lot of logic, which means that any
changes in the top class will break it.

Would you mind to bring the topic up online so that we can figure it
out together?

[6]

+    def raw_execute(self, statement, params):
+        """NOTE: this is being overriden completely because the original

Same case.

[7]

+        # FIXME (even more!): this except/sleep is here because the
+        # storm test suite would have lots of failing tests because
+        # cx_Oracle apparently is not always able to connect several
+        # times in a row, for some reason

Argh!  We have to figure a fix for this somehow.  2 seconds per connection is
a pretty bad limitation. :-(

[8]

+from storm.tracer import debug
+
+DEBUG = True
+
+debug(DEBUG)

All the DEBUG items should be removed from tests before integration
to avoid polluting the natural test output.


...


I guess that's a good start.  Let's try to figure these issues out, and
then I can give another pass if you want.

Thanks for pushing this forward.

-- 
Gustavo Niemeyer
http://niemeyer.net