I had to add the alias to the hash because it was detecting queries as equal that aren't.

A sample of something that breaks (in current code) because of the hash not including aliases is the scalar subquery cache:

SELECT
    (
        SELECT
            1 AS number,
            number
        FROM numbers(1)
    ) AS s,
    (
        SELECT
            1,
            number
        FROM numbers(1)
    ) AS s2

• 22.7.2.15

┌─s─────┬─s2────┐
│ (1,1) │ (1,1) │
└───────┴───────┘

• With these changes:

┌─s─────┬─s2────┐
│ (1,1) │ (1,0) │
└───────┴───────┘

The new one is the correct one, the old one is incorrectly reusing the first scalar result. It's a really odd corner case, but I guess it could happen in real queries.

Now ASTSubquery hash includes the alias which we probably don't want, specially for scalar caches and so on. I'll look into it and maybe remove it for this case only. Needs more thought in any case.

Do we still need the sample block cache?

Do we still need the sample block cache?

The query from the benchmark:

• With the cache and the changes: ~9 QPS ('SampleBlockCacheHit':256,'SampleBlockCacheMiss':15)
• Without the cache: 0.078 QPS, that is 115x worse

So I'd say yes, at least for now.

It seems there are several other places that relied on the alias not being part of the hash, so this needs more work (probably after I come back from holidays)

While looking at the failing tests I've detected some other queries that were working incorrectly because the alias was ignored as part of the node hash. I've added some tests for those queries.

The problem with the failed tests arose from, you guessed it, introducing the alias in the hash, as some parts of the code were implicitly relying on that behaviour. To workaround this I've added some helper methods to calculate other hashes were needed: get the hash of the contents of a subquery (ignoring its alias, id or cte name) to prepare and cache sets, and ignore the alias of functions when processing and optimizing queries based on constraints. I'm thinking that maybe the alias shouldn't be part of the node and be a node on itself, but it's likely better to delay massive changes around that until #23194 is clearer.

In any case, the performance seems still good: master (8.454 QPS) vs PR (9.552 QPS)

Performance improvement seems like the expected 10%:

There are other performance changes but they seem unrelated

Although the query analyzer is being worked on, I still think this PR is worth including: it improves the performance of the sample block cache (and that is not going away I think) and fixes several bugs in the query interpretation.

@kitaisreal said that Sample Block Cache is entirely unneeded if Analyzer is used.

Ok, let's continue on this PR, but I did not review it yet...

@Algunenano no need to continue this pr.
Sample Block cache is only necessary because we have mess and create Interpreters recursively during analyze multiple times. In Analyzer we analyze query only once, and we do not use AST at all.
Methods getContentHash, getTreeHashWithoutAlias, getTreeHash are hard to understand. I expect that there could be complex bugs with distributed processing and aliases that are not covered by our CI/CD.
Profile events SampleBlockCacheHit should not exists because client should not know anything about SampleBlockCache. Client should not know anything about SampleBlock either.

Sample Block cache is only necessary because we have mess and create Interpreters recursively during analyze multiple times. In Analyzer we analyze query only once, and we do not use AST at all.

Great!

Profile events SampleBlockCacheHit should not exists because client should not know anything about SampleBlockCache. Client should not know anything about SampleBlock either.

I don't consider profile events are not only for clients, but also for debugging. Clients doesn't need to know anything about ZK either, but we do when investigating the behaviour of the query.

Anyway, I'm closing the PR and I'll keep (already were) an eye on the analyzer PR to see that the issues found here are fixed by the analyzer too.

@Algunenano thank you!
We need more eyes on the analyzer PR. It is almost ready for review.