Skip to content

Loading…

When joining to a dataset using #with, properly transfer CTEs to the result #16

Merged
2 commits merged into from

2 participants

@jfirebaugh

Tested with spec_core, spec_mssql and spec_integration.

@jeremyevans
Owner

This seems to promote the CTE to the main dataset, which leads to issues with name clashes:

DB[:t].with(:t, DB[:x]).inner_join(DB[:t].with(:t, DB[:y]))

Shouldn't this do a subselect, or is that not allowed on Microsoft SQL Server? The subselect way works fine on PostgreSQL, and is more correct, so I'd only be OK with this method if SQL Server didn't support with in a subselect, and the changes were moved into the mssql shared adapter.

Also, you said you tested it with spec_core, spec_mssql, and spec_integration. I see an additional core spec, but no added spec for mssql or integration. Is this behavior already specified in the mssql or integration tests? If so, it already passes and such a change is not needed. If not, a spec should be added to the mssql and/or integration tests.

@jfirebaugh

I was assuming that name clashes were not something to worry about since the existing code does not check for name clashes when using multiple #withs.

I didn't know that some adapters support CTEs in subselects; SQL Server does not. I agree it should move to mssql adapter.

It looks like there are some adapter specs for CTEs on mssql, I'll see what can be added there.

@jeremyevans
Owner

You are correct that name clashes are left to the user to resolve in most cases. Detecting the clash and raising an exception is OK behavior, but leaving it to the database to raise an exception is fine as well. I just provided the example to show a join that would break on PostgreSQL.

The best place to add a real spec is in the integration specs, where you should just check that the result is correct, not that any particular SQL syntax is used. dataset_test.rb around line 360 would be a good place to add it. You could even join the table to itself to reuse most of the existing code.

John Firebaugh Support CTEs in joined datasets on SQL Server.
SQL Server does not support CTEs on subqueries, so move any CTEs
on joined datasets to the top level. The user is responsible for
resolving any name clashes this may cause.
282ef6e
@jfirebaugh

Please review revised commit (force push).

@jeremyevans
Owner

Looks good. I'll run it through the test suite tomorrow and commit it if there aren't problems. Thanks!

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 9, 2011
  1. Support CTEs in joined datasets on SQL Server.

    John Firebaugh committed
    SQL Server does not support CTEs on subqueries, so move any CTEs
    on joined datasets to the top level. The user is responsible for
    resolving any name clashes this may cause.
  2. Pass along block in join_table overrides.

    John Firebaugh committed
View
8 lib/sequel/adapters/shared/mssql.rb
@@ -266,6 +266,14 @@ def into(table)
clone(:into => table)
end
+ # SQL Server does not support CTEs on subqueries, so move any CTEs
+ # on joined datasets to the top level. The user is responsible for
+ # resolving any name clashes this may cause.
+ def join_table(type, table, expr=nil, table_alias={}, &block)
+ return super unless Dataset === table && table.opts[:with]
+ clone(:with => (opts[:with] || []) + table.opts[:with]).join_table(type, table.clone(:with => nil), expr, table_alias, &block)
+ end
+
# MSSQL uses a UNION ALL statement to insert multiple values at once.
def multi_insert_sql(columns, values)
[insert_sql(columns, LiteralString.new(values.map {|r| "SELECT #{expression_list(r)}" }.join(" UNION ALL ")))]
View
4 lib/sequel/adapters/shared/mysql.rb
@@ -347,10 +347,10 @@ def having(*cond, &block)
# Transforms an CROSS JOIN to an INNER JOIN if the expr is not nil.
# Raises an error on use of :full_outer type, since MySQL doesn't support it.
- def join_table(type, table, expr=nil, table_alias={})
+ def join_table(type, table, expr=nil, table_alias={}, &block)
type = :inner if (type == :cross) && !expr.nil?
raise(Sequel::Error, "MySQL doesn't support FULL OUTER JOIN") if type == :full_outer
- super(type, table, expr, table_alias)
+ super(type, table, expr, table_alias, &block)
end
# Transforms :natural_inner to NATURAL LEFT JOIN and straight to
View
5 spec/adapters/mssql_spec.rb
@@ -177,6 +177,11 @@ def logger.method_missing(m, msg)
@ds2.insert_sql(@db[:t]).should == 'WITH T AS (SELECT * FROM X UNION ALL SELECT * FROM T) INSERT INTO T SELECT * FROM T'
end
+ specify "should move WITH clause on joined dataset to top level" do
+ @db[:s].inner_join(@ds1).sql.should == "WITH T AS (SELECT * FROM X) SELECT * FROM S INNER JOIN (SELECT * FROM T) AS T1"
+ @ds1.inner_join(@db[:s].with(:s, @db[:y])).sql.should == "WITH T AS (SELECT * FROM X), S AS (SELECT * FROM Y) SELECT * FROM T INNER JOIN (SELECT * FROM S) AS T1"
+ end
+
context "on #import" do
before do
@db = @db.clone
View
5 spec/integration/dataset_test.rb
@@ -386,6 +386,11 @@
ps.call(:n=>3).should == [{:id=>5, :parent_id=>3}, {:id=>6, :parent_id=>5}]
ps.call(:n=>5).should == [{:id=>6, :parent_id=>5}]
end
+
+ specify "should support joining a dataset with a CTE" do
+ @ds.inner_join(@db[:t].with(:t, @ds.filter(:parent_id=>nil)), :id => :id).select(:i1__id).order(:i1__id).map(:id).should == [1,2]
+ @db[:t].with(:t, @ds).inner_join(@db[:s].with(:s, @ds.filter(:parent_id=>nil)), :id => :id).select(:t__id).order(:t__id).map(:id).should == [1,2]
+ end
end
end
Something went wrong with that request. Please try again.