Storm

Code review comment for lp:~lifeless/storm/with

with
Merge into trunk

Revision history for this message

Robert Collins (lifeless) wrote on 2011-03-10:

On Fri, Mar 11, 2011 at 2:16 AM, Gustavo Niemeyer <email address hidden> wrote:
> [1]
>
>> we could rename using to from_ (another keyword :( ...) or perhaps
>> store.use_with and store.use_from or something like that.
>
> Doesn't seem significantly better.
>
> The WITH clause seems somewhat similar in essence to an alias, except
> the definition of the alias is more than a simple rename. This means
> that pretty much every location in Storm which could accept a table
> can theoretically be defined with the help of a WITH clause.
>
> I'm wondering if we might come up with a "With" type that could
> optionally be defined name-less (similar to how Alias works), and be
> used anywhere a real table can be.

That would be beautiful - its outside my understanding of the plumbing
- the compiler state in particular - to make that work properly, but I
can certainly add a couple of (possibly) failing tests.

>> We're going to run a fork in LP for the moment - we need this feature
>> to solve performance problems.
>
> Please don't. We know what will happen next.
>
> Launchpad has survived for years without this feature, so please
> wait a bit longer and let's actually sort out the right way to do it.

Launchpad has been terribly slow for years, for a number of reasons,
but one of the key things in fixing the performance issues has been
accepting that we need to solve the problem using the least-wrong
approach that is ready to hand, and *then* refactor to make the
solution have our ideal characteristics. Using this approach we've
unblocked years of inertia and made substantial performance gains for
Launchpad.

> You'll not want to go around patching code once we determine the way
> that is going to be integrated, and will effectively be living with a
> fork forever.

We'll have a couple of callsites at most, and it should be straight
forward to update them. Launchpad has run storm versions that vary
from trunk before and converged on trunk again.

In point of fact, the datetime change that is in trunk revision 386
will be more work for us to update to than a better API for doing
with.

-Rob

On Fri, Mar 11, 2011 at 2:16 AM, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
> [1]
>
>> we could rename using to from_ (another keyword :( ...) or perhaps
>> store.use_with and store.use_from or something like that.
>
> Doesn't seem significantly better.
>
> The WITH clause seems somewhat similar in essence to an alias, except
> the definition of the alias is more than a simple rename.  This means
> that pretty much every location in Storm which could accept a table
> can theoretically be defined with the help of a WITH clause.
>
> I'm wondering if we might come up with a "With" type that could
> optionally be defined name-less (similar to how Alias works), and be
> used anywhere a real table can be.

That would be beautiful - its outside my understanding of the plumbing
- the compiler state in particular - to make that work properly, but I
can certainly add a couple of (possibly) failing tests.

>> We're going to run a fork in LP for the moment - we need this feature
>> to solve performance problems.
>
> Please don't.  We know what will happen next.
>
> Launchpad has survived for years without this feature, so please
> wait a bit longer and let's actually sort out the right way to do it.

> You'll not want to go around patching code once we determine the way
> that is going to be integrated, and will effectively be living with a
> fork forever.

We'll have a couple of callsites at most, and it should be straight
forward to update them. Launchpad has run storm versions that vary
from trunk before and converged on trunk again.

In point of fact, the datetime change that is in trunk revision 386
will be more work for us to update to than a better API for doing
with.

-Rob

« Back to merge proposal