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

tkalkirill · 2026-07-28T12:36:15Z

@@ -84,9 +87,6 @@
 /** Validator. */
 @Value.Enclosing
 public class IgniteSqlValidator extends SqlValidatorImpl {


Maybe we should use SqlTypeName.DECIMAL, similar to the CALCITE-7624?
org.apache.calcite.sql.validate.SqlValidatorImpl#handleOffsetFetch

A question regarding rounding might arise; we can use the Calcite org.apache.calcite.adapter.enumerable.FetchOffsetRoundingPolicy interface, and it will handle rounding when creating nodes.

this issue is an adoption of related ignite-3 issue, let`s move sequentially here and implement it during different activity, also i suppose that Rounding policy need to work equal for aggregates avg too, need additional tests and check that it work the same as in common DB`s

this issue is an adoption of related ignite-3 issue###

Which issue are you referring to?

let`s move sequentially here and implement it during different activity

Why? In this ticket, you're checking how it handles BigDecimal, and it’s not obvious to me how it will behave with floating-point numbers.

Rounding policy need to work equal for aggregates avg too

Rounding within a set of values is one thing, while rounding during calculations is another. That is precisely why the interface is named FetchOffsetRoundingPolicy to avoid conflating the two types of logic.

need additional tests and check that it work the same as in common DB`s

As a Calcite ticket investigation revealed, different DBMSs handle floating-point numbers differently for FETCH, OFFSET, and LIMIT. That is why this interface was introduced.

Which issue are you referring to?
written in initial issue:
https://issues.apache.org/jira/browse/IGNITE-24227

Thanks for the link; I also left some questions.

tkalkirill · 2026-07-28T12:39:55Z

+                Object param = parameters[dynamicParam.getIndex()];
+
+                if (!(param instanceof Number)) {
+                    SqlTypeName expectType = SqlTypeName.BIGINT;


If we support the SqlTypeName.DECIMAL as in Calcite, then it will need to be fixed.

tkalkirill · 2026-07-28T12:41:15Z

+        try {
+            long res = IgniteMath.convertToLongExact(offsetFetchLimit);
+
+            if (res < 0)


If we support the SqlTypeName.DECIMAL as in Calcite, then it will need to be fixed.

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

tkalkirill · 2026-08-04T09:29:21Z

-        this.fetchNode = fetchNode;
+        this.offset = offset;
+        rowsSummary = fetch == -1 ? Long.MAX_VALUE : IgniteMath.addExact(fetch, offset);
+        this.fetch = fetch == -1 ? 0 : fetch;


The documentation says nothing about what 0 means.

change doc for :

/** How many rows need to be processed. */ private final long fetch;

is it ok ?

No, what do you mean by "zero"?

directly how it written in documentation: "How many rows need to be processed"

This description is misleading because fetch == 0 has two opposite meanings: either no rows should be returned when 1FETCH 01 is specified, or all remaining rows should be returned when FETCH is not specified. These two opposite cases should not be represented by the same value.

ok, i tried to fix it ..

The documentation is fine, but replace the duplicated -1 sentinel here:

rowsSummary = fetch == FETCH_DEFAULT ? Long.MAX_VALUE : IgniteMath.addExact(fetch, offset);

Also update the constructor parameter documentation to reference {@link #FETCH_DEFAULT} instead of mentioning the literal -1 directly. This keeps the implementation and documentation tied to the declared constant.

You didn't fix the documentation.

yep, missed, now it`s ok

tkalkirill · 2026-08-04T09:36:58Z

-        @Nullable Supplier<Integer> offset,
-        @Nullable Supplier<Integer> fetch
+        long offset,
+        long fetch


The documentation says nothing about what -1 means.

Use the declared default constants consistently throughout this constructor. Replace the literal -1 in the fetch parameter documentation with {@link #FETCH_DEFAULT} and update the assertion accordingly:

assert fetch == FETCH_DEFAULT || fetch > 0 : "Unexpected fetch = " + fetch;

The delegating constructor should also pass OFFSET_DEFAULT and FETCH_DEFAULT instead of the literal 0 and -1 values.

You didn't fix the documentation.
You didn't fix the second constructor.

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-05T13:03:37Z

     * @param ctx Execution context.
     * @param rowType Row type.
+     * @param offset How many rows need to be skipped.
+     * @param fetch How many rows need to be processed, {@code -1} if param is undefined.


Suggested change

* @param fetch How many rows need to be processed, {@code -1} if param is undefined.

* @param fetch How many rows need to be processed, {@link #FETCH_DEFAULT} if param is undefined.

tkalkirill · 2026-08-05T13:04:05Z

     * @param comp Rows comparator.
     * @param offset Offset.
-     * @param fetch Limit.
+     * @param fetch How many rows need to be processed, {@code -1} if param is undefined.


Suggested change

* @param fetch How many rows need to be processed, {@code -1} if param is undefined.

* @param fetch How many rows need to be processed, {@link #FETCH_DEFAULT} if param is undefined.

done thanks

tkalkirill · 2026-08-05T13:04:43Z

     */
    public SortNode(ExecutionContext<Row> ctx, RelDataType rowType, Comparator<Row> comp) {
-        this(ctx, rowType, comp, null, null);
+        this(ctx, rowType, comp, 0, -1);


Suggested change

this(ctx, rowType, comp, 0, -1);

this(ctx, rowType, comp, OFFSET_DEFAULT, FETCH_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

	* @param fetch How many rows need to be processed, {@code -1} if param is undefined.
	* @param fetch How many rows need to be processed, {@link #FETCH_DEFAULT} if param is undefined.

	this(ctx, rowType, comp, 0, -1);
	this(ctx, rowType, comp, OFFSET_DEFAULT, FETCH_DEFAULT);

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!

Choose a reason for hiding this comment

Uh oh!

zstan Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkalkirill Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

zstan commented Jul 2, 2026 •

edited by tkalkirill

Loading

zstan Aug 3, 2026 •

edited

Loading

tkalkirill Aug 4, 2026 •

edited

Loading

tkalkirill Aug 5, 2026 •

edited

Loading