On Mar 3, 2014, at 6:00 PM, Daniel L. Wang <[log in to unmask]> wrote:

> Hi Serge,
> 
> If you've got a minute, have a look at my join syntax branch: u/danielw/joinSyntax6 . I think it handles the needed syntax for joins, but the code for constructing the representation is biased towards creation from a parse tree. As such, it may be lacking in functionality to create components programmatically.
> 
> I know there is stuff missing. What stands out? Do you need the boolean predicates that are currently unimplemented?  Is there a particular aspect whose cleanup would make your life easier?

Nothing really stands out, and I’m not sure I’ll have more to say before I start actually using these things as part of the Ref*Match ticket. I’m not really very worried about building query representations even if it’s super clunky. I’m much more worried about recognizing all possible surface syntaxes for the same join, which seems like it’s going to real “fun”.

I’m not totally sure what to diff --stat against; I ended up using commit c4b9ea0. I’m still looking at FromFactory and TableStrategy, so there will probably be a few more comments, but here’s an initial set of comments. Let me know which trac ticket they should be attached to (2995?).


 wdb/QuerySql.h and wdb/QuerySql_Batch.h:
 * I don't understand why there is a `QuerySql::Factory` nested class here instead of a `QuerySql` constructor.
 * C-style cast to `unsigned` on line 60 of QuerySql_Batch.h. Consider making `pos` and `batchSize` instances of `QuerySql::StringList::size_type` to avoid the casts.
 * `QuerySql::Batch` is used in one place only - helper functions in `QueryRunner.cc`. Maybe `Batch` could move there? Also, I think it would be a little nicer to make `Batch` hold a const-reference to the input `StringList` and generate batches on demand. If this code will become more broadly useful, then you could could even move to a pair of const forward iterators and make it a generic template.
 * `StringList` is a `std::deque<std::string>` here, but everywhere else, it's a `std::list<std::string>`. Why not `StringDequeue` (if there is some reason to use a double-ended queue here)?

query/TableAlias.h:
 * `DbTablePair`: change `lessThan` to `operator<`, and get rid of `pairLess` in `TableAliasReverse`.
 * Shouldn't `TableAlias::get` be `const`? Maybe make `TableAlias::Map` a private typedef?
 * `TableAliasReverse::get`: it seems easy to make this `const` and retain existing behavior by returning a reference to an empty string if `_map` doesn't contain `p` and the inexact lookup fails.
 * `TableAlias::set` and `TableAliasReverse::set`: check for and reject empty aliases?
 * The FIXME  in `TableAliasReverse::get` worries me. If we're going to leave that unimplemented, shouldn't there be a check that the lookup isn't ambiguous? Also, if you made `DbTablePair::<` sort on (table, db) rather than (db, table), you could use `std::map::lower_bound` to good effect here.

query/TableRef.*
 * `applySimpleRO` doesn't seem to be implemented. Also, why not overload `applySimple`?
 * Consider favoring descriptive names over the current short ones containing one letter abbreviations (`FuncC`, `Pfunc`)? `FuncC` is comprehensible here because its definition happens to be next to `Func`, but if it were used by itself somewhere else, I'd be puzzled. I also don't think it'd be crazy to make `applySimple` a template and omit `Func`/`FuncC` entirely.
 * `Pfunc` should have a virtual destructor.
 * There's no documentation on what  a `Pfunc` is or how to write one. The signature just left me wondering how I was supposed to come up with a list of `TableRef::Ptr`s from a single `TableRef`. I also don't understand why instead of having a (non-virtual) `permute()` that throws a `logic_error`, you don't just omit `permute()`.
 * `clone()`: isn't the FIXME here easy to take care of?

query/Predicate.cc:
 * `NullPredicate::copySyntax()`: `p` should be a `NullPredicate::Ptr` rather than a bare pointer for exception safety. The current implementation leaks memory if `clone()` throws. Note that all the other `copySyntax()` implementations suffer from the same flaw.

query/JoinSpec.*:
 * Looks pretty good. You are a bit inconsistent about checking what seems to be the class invariant (`putTemplate()` checks that you have exactly one of an ON and USING, but `clone()` doesn't). It seems to me you could enforce this entirely in the constructors (by making sure the constructors fail on null pointers). But if you'd rather still be defensive, maybe add a private method to check the invariant?

query/JoinRef.*:
 * Not sure if we care, but a `CROSS JOIN` can't be natural. But a natural inner join on two tables that have no identically named columns is a cross join. And I don't think a natural join can have a join predicate, since NATURAL implies an implicit join predicate by definition (although the SQL 92 BNF doesn't seem to prohibit it syntactically). My preference would be to deal with this by not supporting NATURAL joins at all (as in, not even at the grammar level).
 * The `JoinRef::putStream()` implementation outputs stuff like "<BROKEN JOIN>" if the internal state of the `JoinRef` is bogus. Compare this to `JoinSpec` that throws a `logic_error`... is there one way that should be preferred?
 
query/FromList.*:
 * Maybe put the implementation inside the appropriate namespace block to avoid the `qMaster` namespace alias and a bunch of using statements?
 * You've got `copyDeep()` here, and you are using `clone()` elsewhere. It's probably worth being consistent.
 * I guess each comma separated term in the FROM clause get its own `TableRef`, and if one uses explicit join syntax, then that chain is embedded inside a single `TableRef`?
 * `permute()` makes my head explode. What's a `columnRefMap` (from the docs in the header, also a comment in the implementation file)? Why does a `permute` of a `FromList` result in a list of `FromList`s? Is it actually supposed to return all possible permutations of a `FromList` or something? Why does the result always include an empty `FromList`? And since it seems to be implemented on top of `TableRef::permute()` (which just throws) and nobody calls it, maybe it can be ditched altogether?
 * `FromList::isJoin` returns false if there is a single `TableRef` that contains one or more `JoinRef`s (perhaps by design, but I just wanted to double check).

control/dispatcher.cc (just something I noticed unrelated to this ticket):
 * The C++ standard (17.4.3.2.1) reserves all identifiers containing two consecutive underscores, or a leading underscore followed by a capital letter. The `_ErrMsgStr_` class should therefore be renamed.

qana/QservRestrictorPlugin.cc:
 * Should `getTable` be called via `applySimple()`, so that it doesn't have to iterate over and recurse into table-refs inside join-refs itself?

qana/ScanTablePlugin.cc:
 * Seems like `getPartitioned` should inherit from both `TableRef::Func` and `TableRef::FuncC`.
########################################################################
Use REPLY-ALL to reply to list

To unsubscribe from the QSERV-L list, click the following link:
https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=QSERV-L&A=1