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

add a new visit pass again to fix select short sight #380

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

yokofly
Copy link
Collaborator

@yokofly yokofly commented Dec 4, 2023

PR checklist:

  • Did you run ClangFormat ?
  • Did you separate headers to a different section in existing community code base ?
  • Did you surround proton: starts/ends for new code in existing community code base ?

Please write user-readable short description of the changes:
fix #356

@yokofly yokofly changed the title add a new visit pass again. add a new visit pass again to fix select short sight Dec 4, 2023
@yokofly yokofly marked this pull request as ready for review December 4, 2023 11:11
@yokofly
Copy link
Collaborator Author

yokofly commented Dec 4, 2023

Not ready for merge and need a test I will discuss more details tomorrow with colleagues

@yokofly yokofly marked this pull request as draft December 5, 2023 01:24
@yokofly yokofly requested a review from yl-lisen December 5, 2023 05:01
@yokofly
Copy link
Collaborator Author

yokofly commented Dec 5, 2023

create stream cte1   (  `Id` int32,   `OrderQty` float,   `LastQty` float);
create stream cte2  (  `Id` int32,   `OrderQty` float,   `LastQty` float);

-- correct:
SELECT 
  cte1.LastQty + cte1.OrderQty AS LastQty,
  least(cte1.LastQty, cte2.LastQty) AS OrderQty
FROM 
  cte1
INNER JOIN cte2 ON cte1.Id = cte2.Id;

-- incorrect:
SELECT 
  least(cte1.LastQty, cte2.LastQty) AS OrderQty,
  cte1.LastQty + cte1.OrderQty AS LastQty
FROM 
  cte1
INNER JOIN cte2 ON cte1.Id = cte2.Id;

when we first start scan/parse the least(cte1.LastQty, cte2.LastQty) AS OrderQty, there is no cte1.OrderQty this columns, and the masked_columns does not contain this. then the required_columns do not contain.

NameSet RequiredSourceColumnsData::requiredColumns() const
{
NameSet required;
for (const auto & pr : required_names)
{
const auto & name = pr.first;
String table_name = Nested::extractTableName(name);
/// Tech debt. There's its own logic for ARRAY JOIN columns.
if (array_join_columns.contains(name) || array_join_columns.contains(table_name))
continue;
if (!complex_aliases.contains(name) || masked_columns.contains(name))
required.insert(name);
}
return required;
}

but I cannot understand why we already have to re-scan again but still will have this problem.
or is this re-scan not enough/adequate?

@yokofly yokofly marked this pull request as ready for review December 5, 2023 06:12
@yokofly yokofly requested a review from chenziliang December 5, 2023 06:12
yokofly and others added 4 commits December 6, 2023 20:57
the sql filed shall be small enough, the plain manual loop perf is enough compared with `std::ranges::all_of` but more read-able?
https://quick-bench.com/q/azeLdyzDIqFnBYNokyg4K1HB6os
```
// src/Interpreters/RequiredSourceColumnsData.cpp:10
bool RequiredSourceColumnsData::addColumnAliasIfAny(const IAST & ast)
{
    if (required_names.contains(alias))
        masked_columns.insert(alias);

    complex_aliases.insert(alias);
    return true;
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to create materialized view due to name conflict
1 participant