IGNITE-28844 Calcite. Improve type checking in LIMIT / OFFSET clauses by zstan · Pull Request #13311 · apache/ignite

zstan · 2026-07-02T08:04:17Z

https://issues.apache.org/jira/browse/IGNITE-28844

Copilot

Pull request overview

This PR (“IGNITE-28844 Calcite. Improve type checking in LIMIT / OFFSET clauses”) tightens validation for LIMIT / OFFSET values (including dynamic parameters) and aligns execution nodes to accept long-based fetch/offset values, with accompanying tests.

Changes:

Added stricter validator checks for LIMIT/OFFSET literals and dynamic parameters (numeric-only, long-range, non-negative).
Introduced a planner-test helper (StatementChecker) and added new planner/integration tests for dynamic parameters in LIMIT/OFFSET.
Updated execution nodes (LimitNode, SortNode) and implementor wiring to use long fetch/offset values and centralize “illegal fetch/offset” error reporting.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
modules/calcite/src/test/sql/order/test_limit.test	Adds negative SQL cases for invalid `FETCH FIRST` / `LIMIT` values and expressions.
modules/calcite/src/test/java/org/apache/ignite/testsuites/PlannerTestSuite.java	Registers the new dynamic-parameters planner test in the suite.
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/StatementChecker.java	Adds a new test helper for validating statements/plans (currently has critical correctness gaps).
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/DynamicParametersPlannerTest.java	Adds planner-level tests for dynamic parameter typing/range checks in `LIMIT`/`OFFSET`.
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/AbstractPlannerTest.java	Adds `checkStatement(...)` helper and `PlanChecker` implementation to drive planner validation in tests.
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/DynamicParametersIntegrationTest.java	Adds integration coverage for `LIMIT ?` with integer and `BigDecimal` parameters.
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/LimitExecutionTest.java	Updates execution tests to new `LimitNode`/`SortNode` APIs (currently inconsistent for “unlimited” fetch).
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/IgniteResource.java	Adds new validator error messages for illegal fetch/offset and dynamic parameter type mismatch.
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/IgniteSqlValidator.java	Implements the new fetch/offset validation logic for literals and dynamic params.
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/SortNode.java	Switches limited-sort parameter types to `long` and adjusts bounded/unbounded buffer selection.
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/LimitNode.java	Switches limit/offset parameter types to `long` and updates request/push logic for new semantics.
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/AbstractNode.java	Introduces a shared `NOT_WAITING` sentinel constant.
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/LogicalRelImplementor.java	Wires runtime fetch/offset evaluation and validation into `LimitNode` / `SortNode` construction.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.

tkalkirill · 2026-07-19T07:28:11Z

Hi, regarding https://issues.apache.org/jira/browse/CALCITE-7624, I implemented support for BigDecimal for the OFFSET/FETCH and LIMIT in #13375.

alex-plekhanov · 2026-07-23T14:18:33Z

+    private final long fetch;
+
+    /** Fetch can be unset. */
+    private final boolean fetchUndefined;


Let's add long limit = fetch == -1 ? Long.MAX_VALUE : IgniteMath.addExact(fetch, offset) instead. To execute IgniteMath.addExact(fetch, offset) only once (and not for each request and each push), to simplify hasMoreData (rowsProcessed < limit).

alex-plekhanov · 2026-07-23T14:20:48Z

+    private final boolean fetchUndefined;

    /** Already processed (pushed to upstream) rows count. */
    private int rowsProcessed;


Comparing int with long, can overflow

alex-plekhanov · 2026-07-23T14:22:12Z

-            if (fetchNode == null || (fetchNode != null && rowsProcessed <= fetch + offset))
-                downstream().push(row);
+        if (rowsProcessed >= offset && hasMoreData()) {
+            // this two rows can`t be swapped, cause if all requested rows have been pushed it will trigger further request call.


Comment should be started with an uppercase letter.

alex-plekhanov · 2026-07-23T14:32:25Z

     */
-    private void checkIntegerLimit(SqlNode n, String nodeName) {
+    private void invalidateFetchOffset(@Nullable SqlNode n, String nodeName) {
+        if (n == null) {


Redundant braces

alex-plekhanov · 2026-07-23T15:16:20Z

    }

+    /** */
+    protected <T extends RelNode> void assertPlan(


Redundant method. Used in assertThrows, but it's better to write own assertThrows method with parameters like PlannerContext or PlannerContextBuilder.

alex-plekhanov · 2026-07-23T15:17:59Z

+
+    /** */
+    @SuppressWarnings("ThrowableNotThrown")
+    static void assertThrows(


Let's rewrite to assertThrows(PlannerContext, cls, msg) or assertThrows(PlannerContextBuilder, cls, msg) and use physicalPlan(...) method inside (not assertPlan)

alex-plekhanov · 2026-07-23T15:18:44Z

+        TestPlanningContextBuilder ctxBuilder,
+        Predicate<T> predicate
+    ) throws Exception {
+        IgniteRel plan = physicalPlan(plannerCtx(ctxBuilder.query, ctxBuilder.schemas, ctxBuilder.planListener,


Move plannerCtx(...) to TestPlanningContextBuilder.build() method.

alex-plekhanov · 2026-07-23T15:20:25Z

 import org.apache.ignite.internal.processors.query.calcite.trait.IgniteDistributions;
 import org.apache.ignite.internal.util.typedef.F;
 import org.apache.ignite.internal.util.typedef.internal.CU;
+import org.apache.ignite.testframework.GridTestUtils;


Redundant change

otherwize it resolves as : org.apache.ignite.internal.processors.query.calcite.planner.AbstractPlannerTest#assertThrows

alex-plekhanov · 2026-07-23T15:22:26Z

+        limit = fetch == -1 ? -1 : (fetch > Long.MAX_VALUE - offset ? -1 : fetch + offset);

-        if (limit < 0)
+        if (limit < 1 || limit > Integer.MAX_VALUE)


Can we rich this line with limit == 0? If yes, it's more correct to use `limit <= 1' or add other logic to return empty result

no we can`t add assert

alex-plekhanov · 2026-07-31T07:53:40Z

-        waiting = -1;
+        waiting = NOT_WAITING;

        downstream().end();


requested field check required here. And requested = 0 before downstream().end(). See related problem https://issues.apache.org/jira/browse/IGNITE-28925

ignitetcbot · 2026-08-05T11:37:47Z

TCBot Test Analysis

TeamCity: --> Run :: All Results
Branch: pull/13311/head
Base: master
Tested commit: c3728ce

Possible Blockers (0)

No blockers found.

New Tests (1)

Calcite SQL 2: 1 tests
- IgniteCalciteTestSuite2: DynamicParametersPlannerTest.testLimitOffset - PASSED

tkalkirill · 2026-08-06T05:08:46Z

                idxBndRel.first() ? cmp : cmp.reversed(),
-                null,
-                () -> 1
+                0,


Suggested change

0,

SortNode.OFFSET_DEFAULT,

zstan requested a review from Copilot July 2, 2026 08:06

Copilot started reviewing on behalf of zstan July 2, 2026 08:07 View session

Copilot AI reviewed Jul 2, 2026

View reviewed changes

zstan force-pushed the ignite-28844 branch from e9195ba to aeebff8 Compare July 2, 2026 10:25

zstan requested a review from Copilot July 2, 2026 10:26

Copilot started reviewing on behalf of zstan July 2, 2026 10:26 View session

Copilot AI reviewed Jul 2, 2026

View reviewed changes

zstan force-pushed the ignite-28844 branch from aeebff8 to c9d1e7d Compare July 2, 2026 11:03

zstan commented Jul 2, 2026

View reviewed changes

Comment thread .../test/java/org/apache/ignite/internal/processors/query/calcite/planner/StatementChecker.java Outdated

zstan force-pushed the ignite-28844 branch from d723e74 to 66a3e6d Compare July 7, 2026 10:29

zstan force-pushed the ignite-28844 branch 2 times, most recently from 79554b4 to 6201e62 Compare July 18, 2026 16:50

zstan mentioned this pull request Jul 19, 2026

IGNITE-28896 SQL Calcite: Support BigDecimal for FETCH, OFFSET and LIMIT #13375

Open

alex-plekhanov reviewed Jul 23, 2026

View reviewed changes

tkalkirill reviewed Jul 28, 2026

View reviewed changes

alex-plekhanov reviewed Jul 31, 2026

View reviewed changes

zstan force-pushed the ignite-28844 branch from 9e414d3 to ce41718 Compare August 3, 2026 14:20

zstan requested a review from isapego as a code owner August 3, 2026 14:20

zstan added 9 commits August 3, 2026 17:27

IGNITE-28844 Calcite. Improve type checking in LIMIT / OFFSET clauses

398a2a2

fix test

538fae5

remove checker class

f1f500e

fix

09ba4ed

initial after review

d287a44

fix after review 2

d1bcf54

interm

fee216a

fix

20b4e81

fix

2ed3579

zstan force-pushed the ignite-28844 branch from ce41718 to 2ed3579 Compare August 3, 2026 14:30

protected method

eda2f04

requested

6f59ef6

tkalkirill reviewed Aug 4, 2026

View reviewed changes

zstan added 6 commits August 4, 2026 15:58

fix after review

75eb192

fix after review

b16789b

fix after review

b98bec5

fix after review

6fb9fd9

fix after review

c3728ce

err msg

18e3656

zstan requested a review from tkalkirill August 5, 2026 05:04

zstan added 4 commits August 5, 2026 13:34

NOT_WAITING align

f55604a

constants instead of methods

1ea9a55

fix

df56657

validateFetchOffset javadoc

6106a19

fix

7f08f51

tkalkirill reviewed Aug 5, 2026

View reviewed changes

zstan added 3 commits August 5, 2026 18:53

type of dyn param

943364c

minors

b9caf4d

fix

e4176f9

tkalkirill approved these changes Aug 6, 2026

View reviewed changes

checkstyle

45ae9eb

zstan merged commit 601c69f into apache:master Aug 6, 2026
7 checks passed

Uh oh!

Conversation

zstan commented Jul 2, 2026 • edited by tkalkirill Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tkalkirill commented Jul 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

zstan commented Jul 2, 2026 •

edited by tkalkirill

Loading