Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: clean up error messages in join checks #8580

Merged
merged 1 commit into from
Aug 17, 2016

Conversation

knz
Copy link
Contributor

@knz knz commented Aug 16, 2016

Prior to this patch a join on two anonymous sources would fail
with an ugly message:

pq: table name "{"" "" %!s(bool=false)}" specified more than once

This was not caught previously because the tests had a wildcard
pattern on the generated table name in the error message. This patch
addresses this and a couple related error messages.


This change is Reviewable

@knz
Copy link
Contributor Author

knz commented Aug 16, 2016

(found while answering #8566)

@RaduBerinde
Copy link
Member

:lgtm:


Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


sql/table_join.go, line 472 [r1] (raw file):

      if _, ok := left.info.sourceAliases[alias]; ok {
          t := alias.Table()
          switch t {

this is just an if/else..


Comments from Reviewable

@knz
Copy link
Contributor Author

knz commented Aug 16, 2016

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


sql/table_join.go, line 472 [r1] (raw file):

Previously, RaduBerinde wrote…

this is just an if/else..

True but I found the 4 braces of an if/else overwhelming to read. Do we have a style guideline here?

Comments from Reviewable

@RaduBerinde
Copy link
Member

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


sql/table_join.go, line 472 [r1] (raw file):

Previously, knz (kena) wrote…

True but I found the 4 braces of an if/else overwhelming to read. Do we have a style guideline here?

Are you seriously going to start using `switch` instead of `if/else` because there are two extra braces? :)

Comments from Reviewable

@knz
Copy link
Contributor Author

knz commented Aug 16, 2016

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


sql/table_join.go, line 472 [r1] (raw file):

Previously, RaduBerinde wrote…

Are you seriously going to start using switch instead of if/else because there are two extra braces? :)

When the payload (body) is large enough (in number of characters) in comparison, then yes... Do you see an objection?

Comments from Reviewable

@RaduBerinde
Copy link
Member

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


sql/table_join.go, line 472 [r1] (raw file):

Previously, knz (kena) wrote…

When the payload (body) is large enough (in number of characters) in comparison, then yes...
Do you see an objection?

No objection, I just find it odd.

Comments from Reviewable

@knz
Copy link
Contributor Author

knz commented Aug 16, 2016

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


sql/table_join.go, line 472 [r1] (raw file):

Previously, RaduBerinde wrote…

No objection, I just find it odd.

Ok I just changed it back.

Comments from Reviewable

Prior to this patch a join on two anonymous sources would fail
with an ugly message:

```
pq: table name "{"" "" %!s(bool=false)}" specified more than once
```

This was not caught previously because the tests had a wildcard
pattern on the generated table name in the error message. This patch
addresses this and a couple related error messages.
@knz knz merged commit 0e74e08 into cockroachdb:develop Aug 17, 2016
@knz knz deleted the fix-sql-errmsg branch August 17, 2016 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants