Interpolate feature #35349
Merged
Interpolate feature #35349
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robot-clickhouse
added
doc-alert
pr-feature
|
|
|
this is a preliminary version, some polishing is required |
yakov-olkhovskiy
changed the title
yakov-olkhovskiy
changed the title
yakov-olkhovskiy
force-pushed
the
interpolate-feature
branch
from
9439f29
to
a8e1671
Compare
INTERPOLATE is added
INTERPOLATE is added
INTERPOLATE is added
INTERPOLATE is added
INTERPOLATE fix
INTERPOLATE fix
yakov-olkhovskiy
force-pushed
the
interpolate-feature
branch
from
ce9a6e3
to
5ae6f80
Compare
use range-based for loop
bugfix: check column existence for INTERPOLATE expression target
|
NOTE: we have decided to refactor filling transform itself to simplify everything and possibly optimize |
… are selected or not, some optimization on expressionless INTERPOLATE
wrong merge fix
| @@ -2499,7 +2557,9 @@ void InterpreterSelectQuery::executeWithFill(QueryPlan & query_plan) | |||
| if (fill_descr.empty()) | |||
| return; | |||
|
|
|||
| auto filling_step = std::make_unique<FillingStep>(query_plan.getCurrentDataStream(), std::move(fill_descr)); | |||
| InterpolateDescriptionPtr interpolate_descr = | |||
| getInterpolateDescription(query, source_header, result_header, syntax_analyzer_result->aliases, context); | |||
There was a problem hiding this comment.
Why do we use a source_header here? I think it should be query_plan.getCurrentDataStream().header instead.
SELECT
n,
any(source),
sum(inter) AS inter_s
FROM
(
SELECT
toFloat32(number % 10) AS n,
'original' AS source,
number AS inter
FROM numbers(10)
WHERE (number % 3) = 1
)
GROUP BY n
ORDER BY n ASC WITH FILL FROM 0 TO 11.51 STEP 0.5
INTERPOLATE ( inter_s AS inter_s + 1 )
Query id: c4a37d26-ec08-42dc-9e0b-eee9a4419eb3
0 rows in set. Elapsed: 0.002 sec.
Received exception from server (version 22.4.1):
Code: 47. DB::Exception: Received from localhost:9000. DB::Exception: Missing columns: 'inter_s' while processing query: 'SELECT n, any(source), sum(inter) AS inter_s FROM (SELECT toFloat32(number % 10) AS n, 'original' AS source, number AS inter FROM numbers(10) WHERE (number % 3) = 1) GROUP BY n ORDER BY n ASC WITH FILL FROM 0 TO 11.51 STEP 0.5 INTERPOLATE ( inter_s AS inter_s + 1 )', required columns: 'n' 'inter_s' 'source' 'inter' 'n' 'inter_s' 'source' 'inter'. (UNKNOWN_IDENTIFIER)
There was a problem hiding this comment.
btw, why spaces inside interpolate expression? Let's fix formatting
INTERPOLATE ( inter_s AS inter_s + 1 ) -> INTERPOLATE (inter_s AS inter_s + 1)
There was a problem hiding this comment.
actually I need columns which participate in interpolate expressions - they can be from source_header and result_header, but getCurrentDataStream().header seems the same as result_header
| { | ||
| if (auto * ident = fn->as<ASTIdentifier>()) | ||
| { | ||
| step.addRequiredOutput(ident->getColumnName()); |
There was a problem hiding this comment.
It looks like here we add alias to required output instead of full column name.
At list this does not work:
SELECT
n,
source,
inter + 1 AS inter_p
FROM
(
SELECT
toFloat32(number % 10) AS n,
'original' AS source,
number AS inter
FROM numbers(10)
WHERE (number % 3) = 1
)
ORDER BY n ASC WITH FILL FROM 0 TO 11.51 STEP 0.5
INTERPOLATE ( inter_p AS inter_p + 1 )
Query id: 9c5b264e-0dca-406e-a2c3-7d9dd43f6609
0 rows in set. Elapsed: 0.002 sec.
Received exception from server (version 22.4.1):
Code: 47. DB::Exception: Received from localhost:9000. DB::Exception: Missing columns: 'inter_p' while processing query: 'SELECT n, source, inter + 1 AS inter_p FROM (SELECT toFloat32(number % 10) AS n, 'original' AS source, number AS inter FROM numbers(10) WHERE (number % 3) = 1) ORDER BY n ASC WITH FILL FROM 0 TO 11.51 STEP 0.5 INTERPOLATE ( inter_p AS inter_p + 1 )', required columns: 'n' 'inter_p' 'source' 'inter' 'n' 'inter_p' 'source' 'inter'. (UNKNOWN_IDENTIFIER)
There was a problem hiding this comment.
Maybe we can build actions for interpolate here as well?
Now I think that maybe it would be easier to substitute aliases for interpolate.
Like, fix QueryNormalizer a little bit (add ASTInterpolateElement)
ClickHouse/src/Interpreters/QueryNormalizer.cpp
Lines 251 to 262 in 5a1392a
so that in AST
inter_p AS inter_p + 1 would be rewritten into inter_p AS (inter + 1) + 1
There was a problem hiding this comment.
it's rather I shouldn't add this column as required because it's not a source column...
src/Parsers/ASTInterpolateElement.h
Outdated
| ASTPtr clone() const override | ||
| { | ||
| auto clone = std::make_shared<ASTInterpolateElement>(*this); | ||
| clone->cloneChildren(); |
There was a problem hiding this comment.
hm, I think you need to fix ptr for clone->expr; otherwise it still points to old ast.
There was a problem hiding this comment.
Maybe like here
ClickHouse/src/Parsers/ASTColumnDeclaration.cpp
Lines 10 to 48 in 5a1392a
There was a problem hiding this comment.
did something, but probably still wrong - will revisit tomorrow...
|
|
||
| auto elem = std::make_shared<ASTInterpolateElement>(); | ||
| elem->column = ident->getColumnName(); | ||
| elem->expr = expr; |
There was a problem hiding this comment.
You also need to add expr to children. Right now, when I do EXPLAIN AST, it does not show interpolate child:
EXPLAIN AST
SELECT
n,
source,
inter + 1 AS inter_p
FROM
(
SELECT
toFloat32(number % 10) AS n,
'original' AS source,
number AS inter
FROM numbers(10)
WHERE (number % 3) = 1
)
ORDER BY n ASC WITH FILL FROM 0 TO 11.51 STEP 0.5
INTERPOLATE ( inter_p AS inter_p + 1 )
Query id: a0864985-0068-40ae-91d1-64e528442940
┌─explain──────────────────────────────────────────────┐
│ SelectWithUnionQuery (children 1) │
...
│ ExpressionList (children 1) │
│ InterpolateElement │
└──────────────────────────────────────────────────────┘
|
found by fuzzer |
|
Another one with logical error |
…ASTInterpolateElement::children
|
conversion for nullable column is not fixed yet... |
| @@ -134,7 +135,7 @@ void QueryNormalizer::visit(ASTTablesInSelectQueryElement & node, const ASTPtr & | |||
|
|
|||
| static bool needVisitChild(const ASTPtr & child) | |||
| { | |||
| return !(child->as<ASTSelectQuery>() || child->as<ASTTableExpression>()); | |||
| return !(child->as<ASTSelectQuery>() || child->as<ASTTableExpression>() || child->as<ASTInterpolateElement>()); | |||
There was a problem hiding this comment.
Let's add a comment why do we skip interpolate.
Comment on lines
1344
to
1360
| auto find_columns = [&step, &select](IAST * function) | ||
| { | ||
| auto f_impl = [&step, &select](IAST * fn, auto fi) | ||
| { | ||
| if (auto * ident = fn->as<ASTIdentifier>()) | ||
| { | ||
| if (select.count(ident->getColumnName()) == 0) | ||
| step.addRequiredOutput(ident->getColumnName()); | ||
| return; | ||
| } | ||
| if (fn->as<ASTFunction>() || fn->as<ASTExpressionList>()) | ||
| for (const auto & ch : fn->children) | ||
| fi(ch.get(), fi); | ||
| return; | ||
| }; | ||
| f_impl(function, f_impl); | ||
| }; |
There was a problem hiding this comment.
maybe we can add a small comment here as well
KochetovNicolai
approved these changes
Apr 11, 2022
|
+1 for adding PARTITION BY |
31 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
added INTERPOLATE extension to the ORDER BY ... WITH FILL
closes #34903