Skip to content

[CALCITE-7662] Add expression support for OFFSET#5113

Open
tkalkirill wants to merge 5 commits into
apache:mainfrom
tkalkirill:calcite-7662
Open

[CALCITE-7662] Add expression support for OFFSET#5113
tkalkirill wants to merge 5 commits into
apache:mainfrom
tkalkirill:calcite-7662

Conversation

@tkalkirill

@tkalkirill tkalkirill commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Jira Link

CALCITE-7662

Changes Proposed

This PR introduces support for arithmetic and scalar expressions for "OFFSET", enabling queries to execute similarly to those in other popular DBMSs (PostgreSQL, Oracle, H2, and MS SQL):

SELECT * FROM person OFFSET ? + 1 ROWS;
SELECT * FROM person OFFSET (udf(?)) ROWS;
SELECT * FROM person OFFSET (1 + NVL(?, 10000)) ROWS.

@tkalkirill

tkalkirill commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

@xiedeyantu Can you take a look? I'm unable to add you to the "Reviewers" section for the review.

@xuzifu666

Copy link
Copy Markdown
Member

You could refine the PR title first.

#

!use post
!set outputformat mysql

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there any relevant MySQL validation?

You can explain that the accuracy of the results has been verified against relevant databases.

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.

Is there any relevant MySQL validation?

I modeled it after the neighboring files.
As far as I understand, this only affects the output style or am I mistaken?

You can explain that the accuracy of the results has been verified against relevant databases.

I checked here: https://onecompiler.com/postgresql

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You might need to add all the test cases here to the link so that the reviewer can easily view them, like this: https://onecompiler.com/postgresql/44veytn3y

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.

new HrSchemaBig(),
"select * from (select empid, name from emps where name like 'G%' union select empid, name from emps where name like '%l') order by name limit 3")
.explainContains("EnumerableLimit(fetch=[3])\n"
.explainContains("EnumerableLimitSort(sort0=[$1], dir0=[ASC], fetch=[3])\n"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When modifying a large number of existing tests, it is best to provide a summary of the reasons to help other reviewers understand the situation more easily.

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 noticed an issue at UNION DISTINCT and decided to fix it right away. I suppose that wasn't the right move, as it resulted in too many changes and is confusing. I think I'll fix it later in a separate ticket.

+ "FROM `FOO`\n"
+ "OFFSET ? ROWS\n"
+ "FETCH NEXT ? ROWS ONLY");
// CALCITE-7592: Arithmetic and scalar expressions are allowed within parentheses.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this comment being modified? Is it related to the associated Jira ticket?

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.

The idea is this: previously, expressions with parenthesized were available only for FETCH, but now they are supported for OFFSET as well. To avoid adding another reference to the proposal, I decided simply to remove it.

@tkalkirill tkalkirill changed the title [CALCITE-7662] Wip [CALCITE-7662] Add expression support for OFFSET Jul 21, 2026
@tkalkirill
tkalkirill requested a review from xuzifu666 July 21, 2026 05:34
}
}

private static void unparseOffsetFetchExpression(SqlWriter writer, SqlNode node) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could unparseOffsetExpression and unparseOffsetFetchExpression be consolidated into a single method? Or perhaps unparseFetchUsingAnsi could be refactored? It seems like there is a lot of similar logic involved.

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 didn't manage to merge everything completely, but I did refactor it a bit.

.fails("(?s).*Encountered \"-\".*");
sql("select name from dept order by name offset ^-^1")
.fails("(?s).*Encountered \"-\".*");
sql("select name from dept order by name offset -1").ok();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did this test pass?

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.

Because I changed the validation for OFFSET. But I fixed that and added a check for negative values ​​for literals.

#

!use post
!set outputformat mysql

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You might need to add all the test cases here to the link so that the reviewer can easily view them, like this: https://onecompiler.com/postgresql/44veytn3y

@sonarqubecloud

Copy link
Copy Markdown

@tkalkirill
tkalkirill requested a review from xiedeyantu July 21, 2026 11:21
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.

3 participants