Hey! Thanks for the detailed issue. For clarity, can you provide a SQL example of how you're using this macro? I see how an empty string would make sense, just want to make sure we're fixing your use case properly

cc @aangelisc

Hi @SpencerTorres , thank you for the quick response!

Our current variable mac allows users to select the MAC address, and this column can contain a large number of values (over 100):

SELECT DISTINCT
  mac
FROM 
  dhcp.lease_grafana_filter
WHERE
  time BETWEEN (${__from:date:seconds}-87300) and ${__to:date:seconds}+86500
  AND $__conditionalAll("dhcp_server" IN (${dhcp_server:sqlstring}), $dhcp_server)
  AND $__conditionalAll("event_id" IN (${event_id:sqlstring}), $event_id)
  AND $__conditionalAll("ip" IN (${ip:sqlstring}), $ip)
ORDER BY mac ASC

The mac column is not Nullable, so we expect an empty string '' when it can't be processed or if the value is directly '':

CREATE
...
   `mac` String,
...

We use this variable in our panels to apply a specific filter in the WHERE clause, optimizing the query if the user selects the $__all value. The $__conditionalAll macro is a perfect fit for this, but if the value is '' we want to apply the specific filter:

SELECT
        $__timeInterval(time) as time,
        concat(dhcp_server, ' / ', event_name) as tuple,
        count() as count
        
    FROM dhcp.lease
    WHERE
        $__timeFilter(time)
        AND (dhcp_server in (SELECT arrayJoin([${dhcp_server:sqlstring}])))
        AND $__conditionalAll("dhcp_server" IN (${dhcp_server:sqlstring}), $dhcp_server)
        AND $__conditionalAll("event_id" IN (${event_id:sqlstring}), $event_id)
        AND $__conditionalAll("ip" IN (${ip:sqlstring}), $ip)
        AND $__conditionalAll("mac" IN (${mac:sqlstring}), $mac)
    GROUP BY time, tuple

    ORDER BY time, tuple

I noticed that the '' issue is related to this - #262 . However, IMHO, since CH initializes the columns, I cannot see the reason to treat "empty" values as an $_all case, specially considering that this behaviour only applies to the String types...

Another comment @SpencerTorres , and apologies to reuse this ticket for this:

We recently migrated all our dashboards from Altinity ClickHouse, which required several adjustments to some dashboards due to the implementation of this macro.

Based on my understanding and code review, the idea for the $__conditionalAll macro originates from the Altinity-ClickHouse plugin (#46), but the implementation differs slightly:

• The grafana-clickhouse-datasource replaces the macro with 1=1, limiting its use to WHERE clauses.
• The altinity-clickhouse-datasource, on the other hand, replaces the $conditionalTest macro with an EMPTY ('') value, enabling the creation of conditional query statements beyond just the WHERE clause.

Hi @SpencerTorres , do you need more information?

Nope, makes sense to me!

Might be a breaking change, but #262 should be using $__all so we can support empty strings

Submitted #960

Hi @SpencerTorres , I really appreciate your work on the PR!

However, IMHO, to avoid conflicts with #262 , it might be better to create a new macro to replicate the behavior as the altinity-clickHouse datasource plugin (originally suggested in #46 ). This new macro would replace the $conditionalTest macro with an EMPTY ('') value, allowing for more flexible conditional query statements beyond just the WHERE clause.

Here's an example use case: based on the value of variable $adv_filter, the following UNION statement is executed if the value is set as $__all

SELECT
    exporter as ip_src
FROM grafana_filter
WHERE stamp_updated > now()+3600*50000
$conditionalTest(
    UNION ALL
    SELECT DISTINCT
        ip_src
    FROM min1
    WHERE
      (stamp_updated between toDateTime($from-87300) and toDateTime($to)) 
      $conditionalTest(AND exporter in (SELECT toIPv4(arrayJoin([$exporter]))),$exporter)
      $conditionalTest(AND (iface_in in ($iface) OR iface_out in ($iface) ),$iface)
      $conditionalTest(AND ip_proto IN ($ipproto),$ipproto)
          $conditionalTest(
            AND well_known_port IN (SELECT toUInt16(arrayElement(splitByChar(',', arrayJoin([$wkp])), 1)))
            AND ip_proto IN (SELECT toUInt8(arrayElement(splitByChar(',', arrayJoin([$wkp])), 2)))
            ,$wkp)
      $conditionalTest(AND dscp IN ($dscp),$dscp)
      $conditionalTest(AND appid IN ($nbar),$nbar)
,$adv_filter)

Thanks!

Hi @SpencerTorres , @aangelisc

Since the proposed PR hasn't been merged yet, I wanted to check in and get your thoughts on my suggestion to maintain compatibility with the current $__conditionalAll and do not break with #262

What do you think?

We discussed it the other week in a meeting, and in my opinion, the open PR is the most correct solution. I think a breaking change is acceptable in this case, because the old behavior seems incorrect. Let me know what you think

Thanks for the feedback @SpencerTorres

I completely agree with your points.

I just wanted you to consider the possibility of modifying the current macro to replace 1=1 with an empty string '', allowing more flexibility for users to create conditional parts of the query, as I tried to reflect in the comments. Although, I understand that this would be a breaking change to the current implementation (the WHERE operators would break).

That said, and apologies for this, it’s outside the scope of this issue, so if you agree, I’ll open a new feature request (FR) to implement a new macro that replaces with an empty string, instead of 1=1

Thank you!

@sbengo good idea, lets split this into a new issue as a dedicated feature request 👍