camertron / scuttle-rb

A library for transforming raw SQL statements into ActiveRecord/Arel queries. Ruby wrapper and tests for scuttle-java.
86 stars 2 forks source link

Incorrect Handling of `case when ... end` #2

Closed posgarou closed 8 years ago

posgarou commented 8 years ago

First off, thanks for your work on this amazing tool.

I've noticed that Scuttle doesn't seem to handle CASE WHEN ... END statements properly. Consider the following simplified query (working with postgres):

SELECT game_events.name, sum(CASE WHEN game_events.has_happened THEN 1 ELSE 0 END) total_has_happened
FROM game_events
GROUP BY game_events.name;

This generates the following incorrect Arel mapping:

GameEvent.select(
  [
    GameEvent.arel_table[:name], Arel::Nodes::NamedFunction.new('sum', [0]).as('total_has_happened')
  ]
).group(GameEvent.arel_table[:name])

Since to my knowledge case statements aren't directly supported in Arel (https://github.com/rails/arel/issues/168), it would be helpful if the above instead treated CASE ... END as raw sql.

GameEvent.select(
  [
    GameEvent.arel_table[:name], Arel.sql('CASE WHEN game_events.has_happened THEN 1 ELSE 0 END').sum.as('total_has_happened')
  ]
).group(GameEvent.arel_table[:name])
camertron commented 8 years ago

Thanks for the bug report, @posgarou. The fact that Arel doesn't support CASE statements directly is a bit of a shame, since it appears the WHEN parts can be full SQL expressions. I think your solution of embedding the raw CASE string is probably the only thing we can do for now. Any interest in submitting a PR for this?

posgarou commented 8 years ago

@camertron Sure, I'll give it a shot. I'll try to submit a PR within a week or so. Thanks!

posgarou commented 8 years ago

@camertron Looked into this more tonight, and I'm going to have to bow out on the PR. I didn't realize the change would have to be made in scuttle-java—haven't worked in Java in forever. Sorry!

camertron commented 8 years ago

Alright, that's no problem @posgarou. I'll take a look at it.

camertron commented 8 years ago

Fix deployed.