search

CSNW / sql-bricks

Transparent, Schemaless SQL Generation
http://csnw.github.io/sql-bricks
MIT License
204 stars 25 forks source link

.crossJoin() requires onCriteria, even though it shoudn't #66

Closed sneakertack closed 9 years ago

sneakertack commented 9 years ago

Calling .crossJoin() without an onCriteria gives a "No join criteria" error, even though on most SQL implementations (including the spec) CROSS JOIN should tend to avoid any on/using/natural qualifiers.

I can fix this; however, what's the guideline on when we should throw an error and when we should let things through?

Some test cases are available here, comparing what MySQL, Postgres, and SQL92 (seem to) allow, versus what sql-bricks is capable of returning.

Taking this further, INNER JOIN (and friends) without qualifers seems valid in some implementations as well.

To summarise - when should a qualifier be enforced, and when should it not?

prust commented 9 years ago

@sneakertack: Sorry for the delay responding -- this is a busy week, I should be able to engage more deeply next week.

My instinct is to keep things user-friendly and keep the requirement for INNER JOIN and friends, but drop the requirementn for CROSS JOIN. If someone finds that they really need to do an INNER JOIN without qualifiers, they can fork the library or we can provide an override or something, but I think that for the majority of users the requirement is helpful (I'm guessing 9 times out of 10 it will help devs quickly find an error in their code).

sneakertack commented 9 years ago

No worries, everyone's got day jobs. I've submitted the PRs, feel free to review whenever you wish. :v: