Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backward compatibility problem --> DB::Exception: Parameters to aggregate functions must be literals. Got parameter 'assumeNotNull(identity(_CAST(0.9, 'Nullable(Float64)')) AS _subquery14) AS TUNING'. #59154

Closed
gervarela opened this issue Jan 24, 2024 · 4 comments · Fixed by #62185
Labels
backward compatibility st-hold We've paused the work on issue for some reason

Comments

@gervarela
Copy link

gervarela commented Jan 24, 2024

As we were evaluating an upgrade from 22.11 to a 23.X stable version we have encountered a posible, and unexpected, 'backwards incompatible' change.

We have a quite complex query that uses CTEs. It works well in our production database with version 22.11, but gives an exception in 23.8.x and 23.12.x.

The problem seems to be related the evaluation of constant expressions as literals when using CTEs/Subqueries as parameters to aggregate functions.

This is a much simplified version of the query, but it exhibits the same behaviour and problems.

SELECT *

FROM

(

    WITH

        assumeNotNull((

            SELECT 0.9

        )) AS TUNING,

        ELEMENT_QUERY AS

        (

            SELECT quantiles(TUNING)(1)

        )

    SELECT *

    FROM ELEMENT_QUERY

)

Gives the exception:

Received exception from server (version 23.12.2):

Code: 134. DB::Exception: Received from localhost:9000. DB::Exception: Parameters to aggregate functions must be literals. Got parameter 'assumeNotNull(identity(_CAST(0.9, 'Nullable(Float64)')) AS _subquery14) AS TUNING'. (PARAMETERS_TO
_AGGREGATE_FUNCTIONS_MUST_BE_LITERALS)

We have also tested (previously) on 23.8.X with same results.

Nonetheless, in 22.11 it works perfectly:

SELECT *
FROM
(
    WITH
        assumeNotNull((
            SELECT 0.9
        )) AS TUNING,
        ELEMENT_QUERY AS
        (
            SELECT quantiles(TUNING)(1)
        )
    SELECT *
    FROM ELEMENT_QUERY
)

Query id: 5d142c59-59fb-4b8c-9581-e491fa9f355f

┌─quantiles(TUNING)(1)─┐
│ [1]                  │
└──────────────────────┘

1 row in set. Elapsed: 0.005 sec. 

The problem seems to be particularly related to the usage of CTEs, because a very similar construction like the one below, works well in all versions:

SELECT

    toTypeName(assumeNotNull((

        SELECT 0.9

    ))),

    assumeNotNull((

        SELECT 0.9

    )) AS param,

    quantile(param)(1)
@UnamedRus
Copy link
Contributor

UnamedRus commented Jan 25, 2024

Works in analyzer

SELECT *
FROM
(
    WITH
        assumeNotNull((
            SELECT 0.9
        )) AS TUNING,
        ELEMENT_QUERY AS
        (
            SELECT quantiles(TUNING)(1)
        )
    SELECT *
    FROM ELEMENT_QUERY
) SETTINGS allow_experimental_analyzer=1;

┌─quantiles(TUNING)(1)─┐
│ [1]                  │
└──────────────────────┘

Broken between 23.1 and 23.2 https://fiddle.clickhouse.com/642eee1f-0671-47a7-beb0-c90cef0a8df6

@Algunenano Algunenano added the st-hold We've paused the work on issue for some reason label Jan 25, 2024
@gervarela
Copy link
Author

gervarela commented Jan 25, 2024

Checking in fiddle.clickhouse.com it is also broken in head - 24.1.1.

https://fiddle.clickhouse.com/a2615745-7849-4eb9-8361-691f1943514d

As @UnamedRus said, it is broken from 23.2 in advance.

Or this the current expected behaviour?

@Algunenano
Copy link
Member

The current behaviour is bad but it's unlikely that it will get fixed in the current interpreter. The new analyzer (allow_experimental_analyzer) that will replace it in the short term already fixes the problem.

The problem with the old interpreter did most of the changes on top of the AST which led to lots complications like this one, where a fix somewhere breaks other parts of the query. That being said, if somebody sends a simple fix for it can be accepted; it's just that the effort is being put in the new infrastructure.

@gervarela
Copy link
Author

Thank you for the clarification @Algunenano.

Is there a roadmap or planned date for the rollout of the new analyzer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backward compatibility st-hold We've paused the work on issue for some reason
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants