Positional parameters about clickhouse_ecto

[scarfacedeb]

@santaux I had more time to think about it, and I'm certain now that it's better to keep things as simple as possible in the adapter.

Positional parameters is a nice-to-have feature, but it should be supported by clickhouse itself.

Faking it in the adapter creates the unwanted discrepancy between db implementation, api of other clients (e.g. in other languages) and the current client's api.

[scarfacedeb]

@IvanIvanoff could you provide a code example for ecto query with multiple params with the same ?1 placeholder? I couldn't think of such cases in our code, that's why I though it will be safe to remove positional arguments. (like in mysql adapter)

[IvanIvanoff]

@scarfacedeb For one of my use cases I'm writing plain sql and execute it with ClickhouseRepo.query.
The example shows how to fill gaps when bucketing into time intervals:

query = """
    SELECT SUM(value), time
    FROM (
      SELECT
        toDateTime(intDiv(toUInt32(?4 + number * ?1), ?1) * ?1) as time,
        toFloat64(0) AS value
      FROM numbers(?2)
      UNION ALL
      SELECT toDateTime(intDiv(toUInt32(dt), ?1) * ?1) as time, sum(value) as value
      FROM transfers
      PREWHERE from IN (?3) AND NOT to IN (?3)
      AND dt >= toDateTime(?4)
      AND dt <= toDateTime(?5)
      GROUP BY time
      ORDER BY time
    )
    GROUP BY time
    ORDER BY time
    """
    args = [
      interval,
      span,
      wallets,
      from_datetime_unix,
      to_datetime_unix
    ]

Without positional params I have to pass 11 arguments instead of 5.
Note that with the old behaviour you can achieve this by passing 5 params and fill the rest with nil or any other value (due to the bug I'm fixing in my PR).

[scarfacedeb]

@IvanIvanoff I could see how positional params can be useful here.

But I think that clickhouse_ecto is not the right place to implement them.

The actual substitution of ? params with values is handled by clickhousex in https://github.com/appodeal/clickhousex/blob/a643602777781187966d916278060d4e6073453a/lib/clickhousex/helpers.ex#L5

Therefore modifying bind_query_params to handle more complex ?<number> params would make them also work with clickhouse_ecto (with small adjustments).

@santaux what do you think?

[scarfacedeb]

On second thought, clickhouse itself doesn't support any placeholder params at all.

Therefore any params-related features will be implemented in the clickhousex only, without the db support.

On the one hand, It gives certain flexibility to the implementation. On the other hand, it means extra support for that feature.

In contrast to postgres that supports ordered params out of the box, and therefore it makes sense to use them in postgrex.

I guess only the maintainer can tell if it's worth pursuing.

[IvanIvanoff]

@scarfacedeb I'm still not convinced where it should be handled. clickhouse_ecto translates the query from containing ?<num> to plain query containing plain ? and updates the argument list accordingly. In my opinion clickhousex should implement only syntax that ClickHouse understands.

Are you aware of any such situation in other adapters and how they handle it?
Now when I think about it I'm also not sure if adding features not supported by ClickHouse brings more benefit or more harm.

Edit: Yeah, you managed to post the same thoughts just a second before me.

[scarfacedeb]

@IvanIvanoff

Now when I think about it I'm also not sure if adding features not supported by ClickHouse brings more benefit or more harm.

yep, that's exactly what I was thinking in the message above :)

Are you aware of any such situation in other adapters and how they handle it?

As far as my understanding goes,

postgrex supports positional params, because postgres itself supports them (see: https://github.com/elixir-ecto/postgrex/blob/43aa6b964043dd8726dba363245aebd3734bde47/lib/postgrex/messages.ex#L289)

Ecto.Adapters.Mysql only supports ? params. (and it looks like mariaex sends those params directly: https://github.com/xerions/mariaex/blob/16d8abcfe479005651bd11050303382448664e6a/lib/mariaex/protocol.ex#L487)

[santaux]

@scarfacedeb

what do you think?

I think that you were right when said that clickhouse itself doesn't support any placeholder params at all. And also, as @IvanIvanoff said:

Now when I think about it I'm also not sure if adding features not supported by ClickHouse brings more benefit or more harm.

🤔