Skip to content

IGNITE-28971 SQL Calcite: Introduce value resolver for FETCH, OFFSET, and LIMIT - #13467

Open
tkalkirill wants to merge 3 commits into
masterfrom
ignite-28971
Open

IGNITE-28971 SQL Calcite: Introduce value resolver for FETCH, OFFSET, and LIMIT#13467
tkalkirill wants to merge 3 commits into
masterfrom
ignite-28971

Conversation

@tkalkirill

Copy link
Copy Markdown
Contributor

Comment thread modules/calcite/src/test/sql/order/test_limit.test

@zstan zstan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, plz check my comments i`d prefer to call also @alex-plekhanov as an ideolog of Contexts extensions.

private SqlValidator validator() {
if (validator == null)
validator = new IgniteSqlValidator(operatorTbl, catalogReader, typeFactory, validatorCfg, ctx.parameters());
validator = createSqlValidator(catalogReader);

@zstan zstan Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that it`s all about code unification - but nethertheless it`s strange to pass already existing 'catalogReader' here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix it.

throw new IllegalArgumentException("Negative value for " + op);

return IgniteMath.convertToLongExact(paramAsDecimal, RoundingMode.DOWN);
IgniteSqlPaginationPolicy pagPlc = ctx.unwrap(IgniteSqlPaginationPolicy.class);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let`s add @Nullable to PlanningContext#unwrap ?
Probably it would be clear to unify this code and the equal from IgniteSqlValidator#checkLimitOffset ->
a mean this :

            RoundingMode roundingMode = pagPlc == null ? IgniteMath.NUMERIC_ROUNDING_MODE : pagPlc.roundingMode();
            IgniteMath.convertToLongExact(val, roundingMode);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed all of this.

@zstan
zstan requested a review from alex-plekhanov August 18, 2026 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants