You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "tkalkirill (via GitHub)" <gi...@apache.org> on 2026/07/20 17:44:54 UTC
[PR] [CALCITE-7662] Wip [calcite]
tkalkirill opened a new pull request, #5113:
URL: https://github.com/apache/calcite/pull/5113
<!--
Thanks for sending a pull request! Here are some critical tips for you:
1. READ THE GUIDE FIRST: https://calcite.apache.org/develop/#contributing
*For significant contributions, please discuss on the dev mailing list or Jira BEFORE coding.*
2. JIRA IS USUALLY MANDATORY: Ensure you have created an issue on the Calcite Jira:
https://issues.apache.org/jira/projects/CALCITE/issues
*Check existing issues first to avoid duplicates.*
*Note: A Jira is NOT required for typos and cosmetic changes (i.e., changes that are neither bugs nor features).*
3. TIMING: Strongly recommended to create the Jira BEFORE you start writing code (e.g., a day or so before posting a PR).
This gives others a chance to weigh in on your specification.
4. CRITICAL CONSISTENCY RULE
The following three items MUST match exactly in wording and meaning:
(A) The Jira Issue Title
(B) This Pull Request Title
(C) Your Git Commit Message
Format: [CALCITE-XXXX] <Description>
Example: [CALCITE-0000] Add IF NOT EXISTS clause to CREATE TABLE
Guidelines for a good Title:
- Illustrate using SQL keywords (in ALL-CAPS) rather than Java method names.
- Focus on the specification and user experience, not the implementation.
- Make it clear whether it is a bug or a feature.
- Use words like "should" to indicate desired behavior vs. current behavior (e.g., "Validator should not close model file").
5. REPRODUCTION: If fixing a bug, please provide a concise SQL example or test case to reproduce the issue for a faster review.
6. TESTING: Ensure `./gradlew build` passes and appropriate tests are added/updated.
-->
## Jira Link
[CALCITE-XXXX](https://issues.apache.org/jira/browse/CALCITE-XXXX)
## Changes Proposed
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5086842512
> > I noticed that the issue originated in a previously merged PR #5004 and affects PR #5086, so it doesn't seem to impact the current PR. However, we could wait for that issue to be resolved before merging this one. @tkalkirill Could you continue working on this?
>
> not only #5086, also #5104 ....
Hmm, I suspect the AI tools weren't used properly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5171124030
@xiedeyantu Just to be safe, I rebased onto the fresh `main` to make sure I wouldn't break anything.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Wip [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5025485905
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5113) **Quality Gate passed**
Issues
 [5 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5113&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5113&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5113&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [96.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5113&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5113&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5113)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5163051973
With the merging of #5133, it seems we can proceed with this PR; I don't think this PR is blocked by #5133. Should we move forward with it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #5113:
URL: https://github.com/apache/calcite/pull/5113#discussion_r3620472474
##########
core/src/main/java/org/apache/calcite/sql/SqlDialect.java:
##########
@@ -1081,37 +1081,49 @@ protected static void unparseFetchUsingAnsi(SqlWriter writer, @Nullable SqlNode
writer.startList(SqlWriter.FrameTypeEnum.FETCH);
writer.keyword("FETCH");
writer.keyword("NEXT");
- if (fetch instanceof SqlLiteral
- || fetch instanceof SqlDynamicParam) {
- fetch.unparse(writer, -1, -1);
- } else {
- final SqlWriter.Frame expressionFrame = writer.startList("(", ")");
- if (fetch instanceof SqlCall) {
- writer.getDialect().unparseCall(writer, (SqlCall) fetch, 0, 0);
- } else {
- fetch.unparse(writer, 0, 0);
- }
- writer.endList(expressionFrame);
- }
+ unparseOffsetFetchExpression(writer, fetch);
writer.keyword("ROWS");
writer.keyword("ONLY");
writer.endList(fetchFrame);
}
}
+ private static void unparseOffsetExpression(SqlWriter writer, SqlNode offset) {
+ if (offset instanceof SqlCall) {
+ writer.getDialect().unparseCall(writer, (SqlCall) offset, 0, 0);
+ } else {
+ offset.unparse(writer, 0, 0);
+ }
+ }
+
+ private static void unparseOffsetFetchExpression(SqlWriter writer, SqlNode node) {
Review Comment:
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.
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -10739,14 +10758,12 @@ void testGroupExpressionEquivalenceParams() {
@Test void testNegativeFetchOffsetLimit() {
sql("select name from dept limit ^-^1")
.fails("(?s).*Encountered \"-\".*");
- sql("select name from dept offset ^-^1")
- .fails("(?s).*Encountered \"-\".*");
+ sql("select name from dept offset -1").ok();
sql("select name from dept fetch next ^-^1 rows only")
.fails("(?s).*Encountered \"-\".*");
sql("select name from dept order by name limit ^-^1")
.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();
Review Comment:
Why did this test pass?
##########
core/src/test/resources/sql/offset.iq:
##########
@@ -0,0 +1,186 @@
+# offset.iq
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to you under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+!use post
+!set outputformat mysql
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Wip [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5026203991
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5113) **Quality Gate passed**
Issues
 [5 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5113&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5113&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5113&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [96.1% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5113&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5113&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5113)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Wip [calcite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5026257883
@xiedeyantu Can you take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Wip [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #5113:
URL: https://github.com/apache/calcite/pull/5113#discussion_r3619195304
##########
core/src/test/resources/sql/offset.iq:
##########
@@ -0,0 +1,186 @@
+# offset.iq
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to you under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+!use post
+!set outputformat mysql
Review Comment:
Is there any relevant MySQL validation?
You can explain that the accuracy of the results has been verified against relevant databases.
##########
core/src/test/java/org/apache/calcite/test/enumerable/EnumerableMergeUnionTest.java:
##########
@@ -176,16 +244,14 @@ class EnumerableMergeUnionTest {
tester(false,
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"
Review Comment:
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.
##########
testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java:
##########
@@ -4075,7 +4075,16 @@ void checkPeriodPredicate(Checker checker) {
+ "FROM `FOO`\n"
+ "OFFSET ? ROWS\n"
+ "FETCH NEXT ? ROWS ONLY");
- // CALCITE-7592: Arithmetic and scalar expressions are allowed within parentheses.
Review Comment:
Why is this comment being modified? Is it related to the associated Jira ticket?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #5113:
URL: https://github.com/apache/calcite/pull/5113#discussion_r3619415369
##########
core/src/test/java/org/apache/calcite/test/enumerable/EnumerableMergeUnionTest.java:
##########
@@ -176,16 +244,14 @@ class EnumerableMergeUnionTest {
tester(false,
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"
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5173559436
Yes, I agree. Since you and @iwanttobepowerful previously suggested pausing this PR, I just want to confirm whether the earlier blocking issues still exist; there is no need for everyone to conduct a detailed review of it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5167716820
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5113) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5113&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5113&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5113&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [97.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5113&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5113&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5113)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5173524137
Please wait for @iwanttobepowerful , @xuzifu666 , and @mihaibudiu to see if there are any other issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Wip [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5029769979
You could refine the PR title first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #5113:
URL: https://github.com/apache/calcite/pull/5113#discussion_r3619334353
##########
core/src/test/resources/sql/offset.iq:
##########
@@ -0,0 +1,186 @@
+# offset.iq
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to you under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+!use post
+!set outputformat mysql
Review Comment:
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #5113:
URL: https://github.com/apache/calcite/pull/5113#discussion_r3619433861
##########
testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java:
##########
@@ -4075,7 +4075,16 @@ void checkPeriodPredicate(Checker checker) {
+ "FROM `FOO`\n"
+ "OFFSET ? ROWS\n"
+ "FETCH NEXT ? ROWS ONLY");
- // CALCITE-7592: Arithmetic and scalar expressions are allowed within parentheses.
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #5113:
URL: https://github.com/apache/calcite/pull/5113#discussion_r3621530144
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -10739,14 +10758,12 @@ void testGroupExpressionEquivalenceParams() {
@Test void testNegativeFetchOffsetLimit() {
sql("select name from dept limit ^-^1")
.fails("(?s).*Encountered \"-\".*");
- sql("select name from dept offset ^-^1")
- .fails("(?s).*Encountered \"-\".*");
+ sql("select name from dept offset -1").ok();
sql("select name from dept fetch next ^-^1 rows only")
.fails("(?s).*Encountered \"-\".*");
sql("select name from dept order by name limit ^-^1")
.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();
Review Comment:
Because I changed the validation for OFFSET. But I fixed that and added a check for negative values for literals.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "iwanttobepowerful (via GitHub)" <gi...@apache.org>.
iwanttobepowerful commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5084564115
https://github.com/apache/calcite/pull/5004
plz first handle this
@mihaibudiu 's commit got lost in this PR. Could you revert it and reorganize?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu merged PR #5113:
URL: https://github.com/apache/calcite/pull/5113
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #5113:
URL: https://github.com/apache/calcite/pull/5113#discussion_r3621611178
##########
core/src/test/resources/sql/offset.iq:
##########
@@ -0,0 +1,186 @@
+# offset.iq
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to you under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+!use post
+!set outputformat mysql
Review Comment:
https://onecompiler.com/postgresql/44vv87ue8
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5081970734
@xiedeyantu It’s been over 48 hours and I’ve squashed the commits, please merge it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5086423592
I noticed that the issue originated in a previously merged PR #5004 and affects PR #5086, so it doesn't seem to impact the current PR. However, we could wait for that issue to be resolved before merging this one. @tkalkirill Could you continue working on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5173536535
We don't have enough reviewers to request 4 reviews for every PR.
I will take a look if you think there's something in my area of expertise that you are unsure about.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5082018088
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5113) **Quality Gate passed**
Issues
 [11 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5113&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5113&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5113&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [97.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5113&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5113&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5113)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5033214343
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5113) **Quality Gate passed**
Issues
 [10 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5113&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5113&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5113&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [97.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5113&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5113&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5113)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #5113:
URL: https://github.com/apache/calcite/pull/5113#discussion_r3621535388
##########
core/src/main/java/org/apache/calcite/sql/SqlDialect.java:
##########
@@ -1081,37 +1081,49 @@ protected static void unparseFetchUsingAnsi(SqlWriter writer, @Nullable SqlNode
writer.startList(SqlWriter.FrameTypeEnum.FETCH);
writer.keyword("FETCH");
writer.keyword("NEXT");
- if (fetch instanceof SqlLiteral
- || fetch instanceof SqlDynamicParam) {
- fetch.unparse(writer, -1, -1);
- } else {
- final SqlWriter.Frame expressionFrame = writer.startList("(", ")");
- if (fetch instanceof SqlCall) {
- writer.getDialect().unparseCall(writer, (SqlCall) fetch, 0, 0);
- } else {
- fetch.unparse(writer, 0, 0);
- }
- writer.endList(expressionFrame);
- }
+ unparseOffsetFetchExpression(writer, fetch);
writer.keyword("ROWS");
writer.keyword("ONLY");
writer.endList(fetchFrame);
}
}
+ private static void unparseOffsetExpression(SqlWriter writer, SqlNode offset) {
+ if (offset instanceof SqlCall) {
+ writer.getDialect().unparseCall(writer, (SqlCall) offset, 0, 0);
+ } else {
+ offset.unparse(writer, 0, 0);
+ }
+ }
+
+ private static void unparseOffsetFetchExpression(SqlWriter writer, SqlNode node) {
Review Comment:
I didn't manage to merge everything completely, but I did refactor it a bit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5030316254
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5113) **Quality Gate passed**
Issues
 [10 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5113&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5113&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5113&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [96.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5113&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5113&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5113)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7662] Add expression support for OFFSET [calcite]
Posted by "iwanttobepowerful (via GitHub)" <gi...@apache.org>.
iwanttobepowerful commented on PR #5113:
URL: https://github.com/apache/calcite/pull/5113#issuecomment-5086790547
> I noticed that the issue originated in a previously merged PR #5004 and affects PR #5086, so it doesn't seem to impact the current PR. However, we could wait for that issue to be resolved before merging this one. @tkalkirill Could you continue working on this?
not only #5086, also #5104 ....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org