plausible / ecto_ch

Ecto ClickHouse adapter
MIT License
72 stars 10 forks source link

to_inline_sql/2 doesn't map large numbers to the correct type #187

Closed oceanlewis closed 2 weeks ago

oceanlewis commented 1 month ago

There was recently an addition of some new cases for Ecto.Adapters.ClickHouse.Connection.param_type/1 to handle mapping numbers to Int128/256/etc. This works great when doing the usual Repo.all calls, but calling Ecto.Adapters.ClickHouse.to_inline_sql/2 seems to miss this codepath and won't cast parameterized numbers correctly.

ruslandoga commented 1 month ago

👋 @oceanlewis

Thank you for the report! However, I haven't been able to reproduce it with the test in https://github.com/plausible/ecto_ch/pull/192

Would you be able to provide more details on when inlining fails?

oceanlewis commented 2 weeks ago

Sorry for not circling back around to this. Here's a minimal example I put together to demonstrate the issue:

defmodule ExampleSchema do
  use Ecto.Schema

  @large_id 10_000_000_000_000_000_000_000_000_000_000_000_000

  @primary_key false
  @derive {Jason.Encoder, except: [:__meta__]}
  schema "example_table" do
    field :large_id, Ch, type: "Int128"
  end

  def create_table do
    MyApp.Repo.query!("""
    CREATE OR REPLACE TABLE example_table (
      large_id Int128
    ) ENGINE = MergeTree()
    ORDER BY (large_id)
      AS (SELECT #{@large_id}::Int128)
    """)
  end

  def demonstrate do
    import Ecto.Query

    query =
      from ex in ExampleSchema,
        select: ex.large_id,
        where: ex.large_id == @large_id

    # Generate and runs
    # ```
    # SELECT e0."large_id"
    # FROM "example_table" AS e0
    # WHERE (e0."large_id" = 10000000000000000000000000000000000000)
    # ```
    # which will return 0 rows, because 10000000000000000000000000000000000000 is not cast
    # as an Int128, and is instead  cast as a Float64 with a value of `1e37`
    MyApp.Repo.to_inline_sql(:all, query)
    |> MyApp.Repo.query!()

    # The correct query to generate would have been
    # ```
    # SELECT e0.large_id
    # FROM example_table AS e0
    # WHERE e0.large_id = CAST('10000000000000000000000000000000000000', 'Int128')
    # ```
    # or similar
  end
end
oceanlewis commented 2 weeks ago

So yeah, the tests you've added should be correct, but because ClickHouse itself isn't inferring the types of the numeric values correctly the resulting query is invalid :(

ruslandoga commented 2 weeks ago

Ah, I see. We can add explicit casts no problem! Thank you for following up!