Fix mixed goals conversions crash

[aerosol]

Changes

When goals with custom properties are mixed with goals without custom properties, ecto_ch only checks the first element of an array to infer its type. When goals are ordered such that those without custom props come first in the array, the custom_props_keys array looks like: [[],[],[],['url']] which results with Array(Nothing), which causes ClickHouse to fail with:

Serialization is not implemented for type Nothing: value [[],[],[],['url']] cannot be parsed as Array(Array(Nothing))

Tests

• Automated tests have been added
• This PR does not require tests

Changelog

• Entry has been added to changelog
• This PR does not make a user-facing change

Documentation

• Docs have been updated
• This change does not need a documentation update

Dark mode

• The UI has been tested both in dark and light mode
• This PR does not change the UI

[ukutaht]

Approved too early.

I think there's a logical error still in the conversions calculation that depends on the order in which goals are inserted (and therefore returned from SELECT?).

Anyways, see the failing test below:

a59b9f2

It passes when I move the creation of the Purchase - All goal to be the first goal created in the setup.

Stumbled upon this pretty randomly, wanted to see if things still work when the goal with and without custom props is on the same event_name. The customer where it blew up was using custom props to narrow down the Outbound Link: Click goal to a specific url they care about but it's valid to keep the goal for the generic event as well.

[aerosol]

@ruslandoga 👋 solution in this PR is trying to work around https://github.com/plausible/ecto_ch/blob/9a517fe9eff14c1cfae6fc8e0c6ebfe372b032e6/lib/ecto/adapters/clickhouse/connection.ex#L1234

The desperate fix would be something like:

   defmacro event_goal_join(goal_join_data) do
     quote do
@@ -467,8 +474,8 @@ defmodule Plausible.Stats.SQL.Expression do
             ?,
             ?,
             ?,
-            ?,
-            ?
+            arraySlice(?, 2),
+            arraySlice(?, 2)
           )
         )
         """,
@@ -481,8 +488,14 @@ defmodule Plausible.Stats.SQL.Expression do
         type(^unquote(goal_join_data).event_names_by_type, {:array, :string}),
         type(^unquote(goal_join_data).scroll_thresholds, {:array, :integer}),
         type(^unquote(goal_join_data).indices, {:array, :integer}),
-        type(^unquote(goal_join_data).custom_props_keys, {:array, {:array, :string}}),
-        type(^unquote(goal_join_data).custom_props_values, {:array, {:array, :string}})
+        type(
+          ^[["__TRICK_ECTO_CH__"] | unquote(goal_join_data).custom_props_keys],
+          {:array, {:array, :string}}
+        ),
+        type(
+          ^[["__TRICK_ECTO_CH__"] | unquote(goal_join_data).custom_props_values],
+          {:array, {:array, :string}}
+        )
       )
     end
   end

Do you think you know how to properly fix this at source?

[ruslandoga]

👋 @aerosol

I think it's time to do that TODO.

I'd like to open a PR with something like this, but maybe a bit cleaner:

defp param_type([]), do: "Array(Nothing)"
  
defp param_type([v | vs]) do
  param_type = param_type(v)
    
  if param_type == "Array(Nothing)" do
    param_type(vs)
  else
    ["Array(", param_type, ?)]      
  end
end

[aerosol]

@ukutaht awful workaround implemented to unblock customers. Tracking ecto_ch at plausible/ecto_ch#262