/diesel

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

Explore generating `enable_multi_table_joins!` in `infer_schema!` #1237

Closed
sgrif opened this Issue Oct 6, 2017 · 0 comments

Comments

Assignees
No one assigned
Labels
Projects
None yet
Milestone
  1.0
1 participant
@sgrif
@sgrif
Member

sgrif commented Oct 6, 2017

It's not clear to users when they need to invoke this macro, and the error message they'll get when they need it is not generally helpful. I'd be interested to see how much compile times would be affected if we were to just generate this macro for every combination of tables in infer_schema!. If we were to do this, we should remove the invocation of it from joinable!

@sgrif sgrif added the mentoring available label Oct 6, 2017

@sgrif sgrif added this to the 1.0 milestone Oct 6, 2017

sgrif added a commit that referenced this issue Oct 8, 2017

@sgrif
Infer `enable_multi_table_joins!` for all possible combination of tables 
This macro is basically a workaround for a lack of certain language
features. It's confusing, and unclear to users when they do or do not
need to invoke it. There's no downside to just speculatively adding it
for all tables, so let's do that.

However, for simplicity, I've opted to also remove the code that makes
`JoinTo` imply this. This will actually improve the error messages
(users won't see an error about a missing `JoinTo` implementation when
trying to use a table as a subselect), and makes it more clear when this
macro needs to be invoked, *especially* after we rename it to
`allow_tables_to_appear_in_same_query!`

I've checked the compile times both on crates.io and our own test suite
with this change, and didn't see any noticeable change.

Fixes #1237.
Fixes #1129.
642ac06

sgrif added a commit that referenced this issue Oct 8, 2017

@sgrif
Infer `enable_multi_table_joins!` for all possible combination of tables 
This macro is basically a workaround for a lack of certain language
features. It's confusing, and unclear to users when they do or do not
need to invoke it. There's no downside to just speculatively adding it
for all tables, so let's do that.

However, for simplicity, I've opted to also remove the code that makes
`JoinTo` imply this. This will actually improve the error messages
(users won't see an error about a missing `JoinTo` implementation when
trying to use a table as a subselect), and makes it more clear when this
macro needs to be invoked, *especially* after we rename it to
`allow_tables_to_appear_in_same_query!`

I've checked the compile times both on crates.io and our own test suite
with this change, and didn't see any noticeable change.

Fixes #1237.
Fixes #1129.
4413c5f

sgrif added a commit that referenced this issue Oct 8, 2017

@sgrif
Infer `enable_multi_table_joins!` for all possible combination of tables 
This macro is basically a workaround for a lack of certain language
features. It's confusing, and unclear to users when they do or do not
need to invoke it. There's no downside to just speculatively adding it
for all tables, so let's do that.

However, for simplicity, I've opted to also remove the code that makes
`JoinTo` imply this. This will actually improve the error messages
(users won't see an error about a missing `JoinTo` implementation when
trying to use a table as a subselect), and makes it more clear when this
macro needs to be invoked, *especially* after we rename it to
`allow_tables_to_appear_in_same_query!`

I've checked the compile times both on crates.io and our own test suite
with this change, and didn't see any noticeable change.

Fixes #1237.
Fixes #1129.
Loading status checks…
d8d7786

@sgrif sgrif referenced this issue Oct 8, 2017

Merged

Infer `enable_multi_table_joins!` for all possible combination of tables #1245

sgrif added a commit that referenced this issue Oct 8, 2017

@sgrif
Infer `enable_multi_table_joins!` for all possible combination of tables 
This macro is basically a workaround for a lack of certain language
features. It's confusing, and unclear to users when they do or do not
need to invoke it. There's no downside to just speculatively adding it
for all tables, so let's do that.

However, for simplicity, I've opted to also remove the code that makes
`JoinTo` imply this. This will actually improve the error messages
(users won't see an error about a missing `JoinTo` implementation when
trying to use a table as a subselect), and makes it more clear when this
macro needs to be invoked, *especially* after we rename it to
`allow_tables_to_appear_in_same_query!`

I've checked the compile times both on crates.io and our own test suite
with this change, and didn't see any noticeable change.

Fixes #1237.
Fixes #1129.
Loading status checks…
4933b8e

sgrif added a commit that referenced this issue Oct 9, 2017

@sgrif
Infer `enable_multi_table_joins!` for all possible combination of tables 
This macro is basically a workaround for a lack of certain language
features. It's confusing, and unclear to users when they do or do not
need to invoke it. There's no downside to just speculatively adding it
for all tables, so let's do that.

However, for simplicity, I've opted to also remove the code that makes
`JoinTo` imply this. This will actually improve the error messages
(users won't see an error about a missing `JoinTo` implementation when
trying to use a table as a subselect), and makes it more clear when this
macro needs to be invoked, *especially* after we rename it to
`allow_tables_to_appear_in_same_query!`

I've checked the compile times both on crates.io and our own test suite
with this change, and didn't see any noticeable change.

Fixes #1237.
Fixes #1129.
Loading status checks…
3e21925

sgrif added a commit that referenced this issue Oct 9, 2017

@sgrif
Infer `enable_multi_table_joins!` for all possible combination of tables 
This macro is basically a workaround for a lack of certain language
features. It's confusing, and unclear to users when they do or do not
need to invoke it. There's no downside to just speculatively adding it
for all tables, so let's do that.

However, for simplicity, I've opted to also remove the code that makes
`JoinTo` imply this. This will actually improve the error messages
(users won't see an error about a missing `JoinTo` implementation when
trying to use a table as a subselect), and makes it more clear when this
macro needs to be invoked, *especially* after we rename it to
`allow_tables_to_appear_in_same_query!`

I've checked the compile times both on crates.io and our own test suite
with this change, and didn't see any noticeable change.

Fixes #1237.
Fixes #1129.
Loading status checks…
78e6e51

sgrif added a commit that referenced this issue Oct 9, 2017

@sgrif
Infer `enable_multi_table_joins!` for all possible combination of tables 
This macro is basically a workaround for a lack of certain language
features. It's confusing, and unclear to users when they do or do not
need to invoke it. There's no downside to just speculatively adding it
for all tables, so let's do that.

However, for simplicity, I've opted to also remove the code that makes
`JoinTo` imply this. This will actually improve the error messages
(users won't see an error about a missing `JoinTo` implementation when
trying to use a table as a subselect), and makes it more clear when this
macro needs to be invoked, *especially* after we rename it to
`allow_tables_to_appear_in_same_query!`

I've checked the compile times both on crates.io and our own test suite
with this change, and didn't see any noticeable change.

Fixes #1237.
Fixes #1129.
Loading status checks…
9f4b607

@sgrif sgrif closed this in #1245 Oct 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment