You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2014/08/07 02:03:52 UTC
Review Request 24432: Fix incorrect join type used in LockMapper.xml.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24432/
-----------------------------------------------------------
Review request for Aurora and Maxim Khutornenko.
Bugs: AURORA-640
https://issues.apache.org/jira/browse/AURORA-640
Repository: aurora
Description
-------
Fix incorrect join type used in LockMapper.xml.
Diffs
-----
src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681
src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e
src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec
Diff: https://reviews.apache.org/r/24432/diff/
Testing
-------
Modified a test case to point out the issue.
Thanks,
Bill Farner
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
Posted by Bill Farner <wf...@apache.org>.
> On Aug. 7, 2014, 12:05 a.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, line 63
> > <https://reviews.apache.org/r/24432/diff/1/?file=654349#file654349line63>
> >
> > How about INNER JOIN instead to be totally explicit?
You and Kevin should has this one out.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24432/#review49834
-----------------------------------------------------------
On Aug. 7, 2014, 12:03 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24432/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 12:03 a.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-640
> https://issues.apache.org/jira/browse/AURORA-640
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix incorrect join type used in LockMapper.xml.
>
>
> Diffs
> -----
>
> src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681
> src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e
> src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec
>
> Diff: https://reviews.apache.org/r/24432/diff/
>
>
> Testing
> -------
>
> Modified a test case to point out the issue.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
Posted by Kevin Sweeney <ke...@apache.org>.
> On Aug. 6, 2014, 5:05 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, line 63
> > <https://reviews.apache.org/r/24432/diff/1/?file=654349#file654349line63>
> >
> > How about INNER JOIN instead to be totally explicit?
>
> Bill Farner wrote:
> You and Kevin should has this one out.
>
> Bill Farner wrote:
> hash*
>
> Maxim Khutornenko wrote:
> Just don't see why we would use "LEFT OUTER JOIN" in some cases but "JOIN" in others. Why second guess what "JOIN" defaults to (INNER, NATURAL or CROSS)?
>
> Kevin Sweeney wrote:
> Personally I prefer JOIN over INNER JOIN since a modifier calls out to me that there's something weird going on here and I need to go draw Venn diagrams. Happy to defer, but let's standardize on one style everywhere.
In fact, for inner joins I prefer avoiding the JOIN keyword altogether like
SELECT (...) FROM A a, B b WHERE a.b_id = b.id AND (...)
A CROSS JOIN is just:
SELECT (...) FROM A a, B b WHERE (...)
And a NATURAL JOIN (or any other type of "strange" join) is just:
SELECT (...) FROM A a NATURAL JOIN B b WHERE (...)
SELECT (...) FROM A a (...) JOIN B b ON a.b_id WHERE (...)
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24432/#review49834
-----------------------------------------------------------
On Aug. 6, 2014, 5:03 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24432/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2014, 5:03 p.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-640
> https://issues.apache.org/jira/browse/AURORA-640
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix incorrect join type used in LockMapper.xml.
>
>
> Diffs
> -----
>
> src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681
> src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e
> src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec
>
> Diff: https://reviews.apache.org/r/24432/diff/
>
>
> Testing
> -------
>
> Modified a test case to point out the issue.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 7, 2014, 12:05 a.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, line 63
> > <https://reviews.apache.org/r/24432/diff/1/?file=654349#file654349line63>
> >
> > How about INNER JOIN instead to be totally explicit?
>
> Bill Farner wrote:
> You and Kevin should has this one out.
>
> Bill Farner wrote:
> hash*
Just don't see why we would use "LEFT OUTER JOIN" in some cases but "JOIN" in others. Why second guess what "JOIN" defaults to (INNER, NATURAL or CROSS)?
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24432/#review49834
-----------------------------------------------------------
On Aug. 7, 2014, 12:03 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24432/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 12:03 a.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-640
> https://issues.apache.org/jira/browse/AURORA-640
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix incorrect join type used in LockMapper.xml.
>
>
> Diffs
> -----
>
> src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681
> src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e
> src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec
>
> Diff: https://reviews.apache.org/r/24432/diff/
>
>
> Testing
> -------
>
> Modified a test case to point out the issue.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
Posted by Kevin Sweeney <ke...@apache.org>.
> On Aug. 6, 2014, 5:05 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, line 63
> > <https://reviews.apache.org/r/24432/diff/1/?file=654349#file654349line63>
> >
> > How about INNER JOIN instead to be totally explicit?
>
> Bill Farner wrote:
> You and Kevin should has this one out.
>
> Bill Farner wrote:
> hash*
>
> Maxim Khutornenko wrote:
> Just don't see why we would use "LEFT OUTER JOIN" in some cases but "JOIN" in others. Why second guess what "JOIN" defaults to (INNER, NATURAL or CROSS)?
Personally I prefer JOIN over INNER JOIN since a modifier calls out to me that there's something weird going on here and I need to go draw Venn diagrams. Happy to defer, but let's standardize on one style everywhere.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24432/#review49834
-----------------------------------------------------------
On Aug. 6, 2014, 5:03 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24432/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2014, 5:03 p.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-640
> https://issues.apache.org/jira/browse/AURORA-640
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix incorrect join type used in LockMapper.xml.
>
>
> Diffs
> -----
>
> src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681
> src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e
> src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec
>
> Diff: https://reviews.apache.org/r/24432/diff/
>
>
> Testing
> -------
>
> Modified a test case to point out the issue.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 7, 2014, 12:05 a.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, line 63
> > <https://reviews.apache.org/r/24432/diff/1/?file=654349#file654349line63>
> >
> > How about INNER JOIN instead to be totally explicit?
>
> Bill Farner wrote:
> You and Kevin should has this one out.
>
> Bill Farner wrote:
> hash*
>
> Maxim Khutornenko wrote:
> Just don't see why we would use "LEFT OUTER JOIN" in some cases but "JOIN" in others. Why second guess what "JOIN" defaults to (INNER, NATURAL or CROSS)?
>
> Kevin Sweeney wrote:
> Personally I prefer JOIN over INNER JOIN since a modifier calls out to me that there's something weird going on here and I need to go draw Venn diagrams. Happy to defer, but let's standardize on one style everywhere.
>
> Kevin Sweeney wrote:
> In fact, for inner joins I prefer avoiding the JOIN keyword altogether like
>
> SELECT (...) FROM A a, B b WHERE a.b_id = b.id AND (...)
>
> A CROSS JOIN is just:
>
> SELECT (...) FROM A a, B b WHERE (...)
>
> And a NATURAL JOIN (or any other type of "strange" join) is just:
>
> SELECT (...) FROM A a NATURAL JOIN B b WHERE (...)
> SELECT (...) FROM A a (...) JOIN B b ON a.b_id WHERE (...)
| SELECT (...) FROM A a, B b WHERE a.b_id = b.id AND (...)
This is a pre SQL-92 syntax that is not compliant with the most recent SQL standards. E.g.: http://stackoverflow.com/questions/1599050/ansi-vs-non-ansi-sql-join-syntax
In addition to the above, I don't see why I should be rewriting the query just to change from INNER JOIN to LEFT OUTER JOIN.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24432/#review49834
-----------------------------------------------------------
On Aug. 7, 2014, 12:03 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24432/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 12:03 a.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-640
> https://issues.apache.org/jira/browse/AURORA-640
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix incorrect join type used in LockMapper.xml.
>
>
> Diffs
> -----
>
> src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681
> src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e
> src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec
>
> Diff: https://reviews.apache.org/r/24432/diff/
>
>
> Testing
> -------
>
> Modified a test case to point out the issue.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
Posted by Bill Farner <wf...@apache.org>.
> On Aug. 7, 2014, 12:05 a.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, line 63
> > <https://reviews.apache.org/r/24432/diff/1/?file=654349#file654349line63>
> >
> > How about INNER JOIN instead to be totally explicit?
>
> Bill Farner wrote:
> You and Kevin should has this one out.
hash*
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24432/#review49834
-----------------------------------------------------------
On Aug. 7, 2014, 12:03 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24432/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 12:03 a.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-640
> https://issues.apache.org/jira/browse/AURORA-640
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix incorrect join type used in LockMapper.xml.
>
>
> Diffs
> -----
>
> src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681
> src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e
> src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec
>
> Diff: https://reviews.apache.org/r/24432/diff/
>
>
> Testing
> -------
>
> Modified a test case to point out the issue.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
Posted by Bill Farner <wf...@apache.org>.
> On Aug. 7, 2014, 12:05 a.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, line 63
> > <https://reviews.apache.org/r/24432/diff/1/?file=654349#file654349line63>
> >
> > How about INNER JOIN instead to be totally explicit?
>
> Bill Farner wrote:
> You and Kevin should has this one out.
>
> Bill Farner wrote:
> hash*
>
> Maxim Khutornenko wrote:
> Just don't see why we would use "LEFT OUTER JOIN" in some cases but "JOIN" in others. Why second guess what "JOIN" defaults to (INNER, NATURAL or CROSS)?
>
> Kevin Sweeney wrote:
> Personally I prefer JOIN over INNER JOIN since a modifier calls out to me that there's something weird going on here and I need to go draw Venn diagrams. Happy to defer, but let's standardize on one style everywhere.
>
> Kevin Sweeney wrote:
> In fact, for inner joins I prefer avoiding the JOIN keyword altogether like
>
> SELECT (...) FROM A a, B b WHERE a.b_id = b.id AND (...)
>
> A CROSS JOIN is just:
>
> SELECT (...) FROM A a, B b WHERE (...)
>
> And a NATURAL JOIN (or any other type of "strange" join) is just:
>
> SELECT (...) FROM A a NATURAL JOIN B b WHERE (...)
> SELECT (...) FROM A a (...) JOIN B b ON a.b_id WHERE (...)
>
> Maxim Khutornenko wrote:
>
> | SELECT (...) FROM A a, B b WHERE a.b_id = b.id AND (...)
>
> This is a pre SQL-92 syntax that is not compliant with the most recent SQL standards. E.g.: http://stackoverflow.com/questions/1599050/ansi-vs-non-ansi-sql-join-syntax
>
> In addition to the above, I don't see why I should be rewriting the query just to change from INNER JOIN to LEFT OUTER JOIN.
Kevin wins by force of comment length. (read: i'm going to commit to fix this up, we can decide on the style async.)
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24432/#review49834
-----------------------------------------------------------
On Aug. 7, 2014, 12:03 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24432/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 12:03 a.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-640
> https://issues.apache.org/jira/browse/AURORA-640
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix incorrect join type used in LockMapper.xml.
>
>
> Diffs
> -----
>
> src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681
> src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e
> src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec
>
> Diff: https://reviews.apache.org/r/24432/diff/
>
>
> Testing
> -------
>
> Modified a test case to point out the issue.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 24432: Fix incorrect join type used in LockMapper.xml.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24432/#review49834
-----------------------------------------------------------
Ship it!
src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml
<https://reviews.apache.org/r/24432/#comment87223>
How about INNER JOIN instead to be totally explicit?
- Maxim Khutornenko
On Aug. 7, 2014, 12:03 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24432/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 12:03 a.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-640
> https://issues.apache.org/jira/browse/AURORA-640
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix incorrect join type used in LockMapper.xml.
>
>
> Diffs
> -----
>
> src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml deb25bddd826ba8d5fc3af6f7b853fba8d59e681
> src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 9e1f8e683697b1af1543be332c6da550e547971e
> src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 283fc7ea1c4c5c102c34894a5b3bc4d828c002ec
>
> Diff: https://reviews.apache.org/r/24432/diff/
>
>
> Testing
> -------
>
> Modified a test case to point out the issue.
>
>
> Thanks,
>
> Bill Farner
>
>