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-23T14:39:16Z

     * @param nodeName Node name.
     */
-    private void checkIntegerLimit(SqlNode n, String nodeName) {
+    private void invalidateFetchOffset(@Nullable SqlNode n, String nodeName) {


invalidate - means make something invalid. Here we validate parameters, let's use validateor check.

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

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

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.

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.

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 added 2 commits July 17, 2026 15:55

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

02859d4

fix test

d066d4a

zstan force-pushed the ignite-28844 branch from 66a3e6d to d066d4a Compare July 17, 2026 12:55

remove checker class

6201e62

zstan force-pushed the ignite-28844 branch 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

fix

1d8b892

alex-plekhanov reviewed Jul 23, 2026

View reviewed changes

Merge branch 'master' into ignite-28844

9e414d3

tkalkirill reviewed Jul 28, 2026

View reviewed changes

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zstan commented Jul 2, 2026 •

edited by tkalkirill

Loading