IGNITE-14777 window functions support#12096
Conversation
oleg-zinovev
changed the title
oleg-zinovev
changed the title
9b522ec to
f04ed9f
Compare
f04ed9f to
dbf1e84
Compare
|
@oleg-zinovev, I've partially reviewed your PR. Review not completed yet, but I have some comments to publish. Also there are a lot of codestyle violations. Please read the https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines article about Ignite codestyle. Most of the problems can be detected automatically (for example using command:
|
| partition = partitionFactory.get(); | ||
| } | ||
| else if (prevRow != null && partCmp != null && partCmp.compare(prevRow, row) != 0) { | ||
| partition.drainTo(rowFactory, outBuf); |
There was a problem hiding this comment.
- This operation can block the thread for a long time.
- Large amount of rows can be stored in outBuf, in worth case there will be 2x input rows count (in partition and in outBuf)
Consider pushing directly to downstream. Drain (and push) only requested amount and postpone next pushes until next request.
There was a problem hiding this comment.
Let's fix it with another ticket, it's not a blocker but complicate the implementation
This comment was marked as outdated.
This comment was marked as outdated.
dbf1e84 to
9f135fa
Compare
This comment was marked as outdated.
This comment was marked as outdated.
…g into window partition factory call, window exclusion validation
9f135fa to
13dd848
Compare
| else { | ||
| Row offsetRow = frame.get(idx); | ||
| Object val = get(0, offsetRow); | ||
| if (val == null) { |
There was a problem hiding this comment.
Based on the description of LAG/LEAD function (and other database behavior), we have to return a value even if it is NULL. A default value returns only in the case when a row does not exist.
statement ok
CREATE TABLE t_lag_lead(id INTEGER, val INTEGER);
statement ok
INSERT INTO t_lag_lead VALUES (1, 10), (2, NULL), (3, 30);
query IIII
SELECT id, val,
LAG(val, 1, 999) OVER (ORDER BY id),
LEAD(val, 1, 999) OVER (ORDER BY id)
FROM t_lag_lead
ORDER BY id;
----
1 10 999 NULL
2 NULL 10 30
3 30 NULL 999
There was a problem hiding this comment.
Hi.
Fixed except the following:
If the third argument of lag/lead is non-nullable, Calcite changes the return type of the function to non-nullable (org.apache.calcite.sql.fun.SqlLeadLagAggFunction#transformType).
Because of this, your example returns 0 instead of null.
Not entirely sure what the best way to handle this.
There was a problem hiding this comment.
Probably we have to rewrite (add CAST: LAG(val, 1, 999) <=> LAG(val, 1, CAST(999 AS INTEGER)) in the rule or something else; I didn't test it) to support expected behavior. But anyway it is not a crucial point; we can do it in the future.
If the fix is not trivial, just leave a comment (TODO that pointed out of the specific JIRA ticket).
The Ignite community can fix it in the future.
There was a problem hiding this comment.
If you write such a CAST directly in the query, nothing changes (org.apache.ignite.internal.processors.query.calcite.prepare.IgniteTypeCoercion#syncAttributes receives two non-nullable data types).
At the same time, inferReturnType is called only during query validation and conversion from SqlNode to RelNode.
I added a custom implementation with overridden inferReturnType.
|
@oleg-zinovev, sorry for delay with review. |
This comment was marked as outdated.
This comment was marked as outdated.
# Conflicts: # modules/calcite/src/test/java/org/apache/ignite/testsuites/ExecutionTestSuite.java # modules/calcite/src/test/java/org/apache/ignite/testsuites/IntegrationTestSuite.java
1a3964c to
7285f91
Compare
This comment was marked as outdated.
This comment was marked as outdated.
…ing in range frame, consider out buf size in window execution
@alex-plekhanov |
Unfortunately, not all code style rules are checked automatically. We have checkstyle maven plugin, which is executed on each build if |
Hi! I ran the check via Maven and “Inspect Code” in IDEA. |
51d530a to
5d2e049
Compare
|
Looks good to me! |
|
@alex-plekhanov I have an issue with tcbot:
Should i request permissions or something else via dev mail list? P.S. https://cwiki.apache.org/confluence/spaces/IGNITE/pages/93325163/Apache+Ignite+Teamcity+Bot#ApacheIgniteTeamcityBot-Q1.BuildLaunchproblems-Forbidden - does not help P.P.S. I manage to run Run::All via ci2.ignite directly and cancel it. |
|
@oleg-zinovev is "Put builds at top of queue" flag unchecked when you trigger build via TC bot? |
Yep |
|
@oleg-zinovev we've added this permission to all registered users. Can you please retry? |
Hi. I can now run the build via tcbot. Thanks for the fix! |
|
Hi, Thanks a lot for the review and your patience! 😊 Sorry for the delay — I’ve been busy the last couple of days; I was planning to finish up the tcbot procedures tomorrow. Thanks for helping with that! The PR had some comments regarding the WindowNode implementation — specifically around memory consumption calculation and potential long‑lasting locks. I’d like to propose a solution for these issues. Could you please advise on the best way to do this? Should I create a separate issue and then open a new PR with the fixes? P.S. If my solution turns out to be unsuitable, I can just close it to avoid wasting your time. |
|
Thanks for your patch! For futher WindowNode improvements please open a new ticket and create PR linked to this ticket. Any your contribution is appreciated. |
3 participants
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see TC.Bot: Check PR)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.
Description of Changes:
Added support for planning and executing window functions.
Added special window functions: row_number, rank, dense_rank, percent_rank, cume_dist, ntile, nth_value, first_value, last_value, lag, lead.
Provides two modes of window function execution:
Supports specifying integer offsets in either variant (ROWS / RANGE) and time interval offsets for RANGE.
Window Planning:
During query planning, windows are split into separate rels for each group of aggregation functions. Each logical window rel includes a collation that is required for correctly partitioning rows and defining frames when computing the window.
Splitting is done using Calcite’s standard rule, which groups function calls based on the window specification (according to the OVER clause).
After that, constants used in the window are projected to support referencing them in the current implementation of Ignite aggregates. (If constants are used in FOLLOWING/PRECEDING, they are directly substituted into the offset, which helps reduce the number of frame boundary searches.)
An additional planning phase was introduced specifically for window planning. (I couldn't find a suitable existing place for the new rules, so I followed the approach used in Apache Drill.)
Separate Change:
During development, when attempting to upgrade to Calcite 1.39, it was discovered that IgniteTypeFactory#leastRestrictive does not take into account the nullability of the resulting type when merging FLOAT and DOUBLE.
P.S. I'll be appreciate to any feedback