You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/02/20 04:17:44 UTC

[GitHub] [incubator-kyuubi] link3280 opened a new pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

link3280 opened a new pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938


   <!--
   Thanks for sending a pull request!
   
   Here are some tips for you:
     1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/contributions.html
     2. If the PR is related to an issue in https://github.com/apache/incubator-kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
     3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
   -->
   
   ### _Why are the changes needed?_
   
   Currently, Flink engine would pull all result rows into memory before returning it to the client. This would be problematic for large result sets and infinite result sets.
   
   This is a sub-task of KPIP-2 https://github.com/apache/incubator-kyuubi/issues/1322.
   
   ### _How was this patch tested?_
   - [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible
   
   - [ ] Add screenshots for manual tests if appropriate
   
   - [x] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request
   


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] SteNicholas commented on a change in pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#discussion_r811585307



##########
File path: externals/kyuubi-flink-sql-engine/src/test/scala/org/apache/kyuubi/engine/flink/operation/FlinkOperationSuite.scala
##########
@@ -761,4 +761,18 @@ class FlinkOperationSuite extends WithFlinkSQLEngine with HiveJDBCTestHelper {
         .getStringVal.getValues.get(0) === "tmp.hello")
     }
   }
+
+  test("ensure result max rows") {
+    withSessionConf()(Map(KyuubiConf.ENGINE_FLINK_MAX_ROWS.key -> "200"))(Map.empty) {
+      withJdbcStatement() { statement =>
+        statement.execute("create table tbl_src (a bigint) with ('connector' = 'datagen')")
+        val resultSet = statement.executeQuery(s"select a from tbl_src")
+        var rows = 0
+        while (resultSet.next()) {
+          rows += 1
+        }
+        assert(rows == 200)

Review comment:
       ```suggestion
           assert(rows === 200)
   ```




-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#issuecomment-1046278968


   # [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1938](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1b95822) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/f25e5c93b8848524927f8e0c08e3d533a745a580?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f25e5c9) will **increase** coverage by `0.01%`.
   > The diff coverage is `94.44%`.
   
   > :exclamation: Current head 1b95822 differs from pull request most recent head 80020ce. Consider uploading reports for the commit 80020ce to get more accurate results
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/graphs/tree.svg?width=650&height=150&src=pr&token=925D4tb9AH&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1938      +/-   ##
   ============================================
   + Coverage     60.89%   60.90%   +0.01%     
     Complexity       69       69              
   ============================================
     Files           302      302              
     Lines         14797    14809      +12     
     Branches       1915     1915              
   ============================================
   + Hits           9010     9019       +9     
     Misses         5020     5020              
   - Partials        767      770       +3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...ine/flink/operation/FlinkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRmxpbmtTUUxPcGVyYXRpb25NYW5hZ2VyLnNjYWxh) | `86.11% <80.00%> (-1.39%)` | :arrow_down: |
   | [...uubi/engine/flink/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `81.81% <100.00%> (+0.73%)` | :arrow_up: |
   | [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `96.46% <100.00%> (+0.16%)` | :arrow_up: |
   | [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `92.50% <0.00%> (-2.50%)` | :arrow_down: |
   | [...n/scala/org/apache/kyuubi/KyuubiSQLException.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9LeXV1YmlTUUxFeGNlcHRpb24uc2NhbGE=) | `92.77% <0.00%> (-1.21%)` | :arrow_down: |
   | [...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvY0J1aWxkZXIuc2NhbGE=) | `83.48% <0.00%> (-0.92%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f25e5c9...80020ce](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#issuecomment-1046278968


   # [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1938](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1b95822) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/f25e5c93b8848524927f8e0c08e3d533a745a580?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f25e5c9) will **increase** coverage by `0.01%`.
   > The diff coverage is `94.44%`.
   
   > :exclamation: Current head 1b95822 differs from pull request most recent head 80020ce. Consider uploading reports for the commit 80020ce to get more accurate results
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/graphs/tree.svg?width=650&height=150&src=pr&token=925D4tb9AH&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1938      +/-   ##
   ============================================
   + Coverage     60.89%   60.90%   +0.01%     
     Complexity       69       69              
   ============================================
     Files           302      302              
     Lines         14797    14809      +12     
     Branches       1915     1915              
   ============================================
   + Hits           9010     9019       +9     
     Misses         5020     5020              
   - Partials        767      770       +3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...ine/flink/operation/FlinkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRmxpbmtTUUxPcGVyYXRpb25NYW5hZ2VyLnNjYWxh) | `86.11% <80.00%> (-1.39%)` | :arrow_down: |
   | [...uubi/engine/flink/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `81.81% <100.00%> (+0.73%)` | :arrow_up: |
   | [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `96.46% <100.00%> (+0.16%)` | :arrow_up: |
   | [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `92.50% <0.00%> (-2.50%)` | :arrow_down: |
   | [...n/scala/org/apache/kyuubi/KyuubiSQLException.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9LeXV1YmlTUUxFeGNlcHRpb24uc2NhbGE=) | `92.77% <0.00%> (-1.21%)` | :arrow_down: |
   | [...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvY0J1aWxkZXIuc2NhbGE=) | `83.48% <0.00%> (-0.92%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f25e5c9...80020ce](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] SteNicholas commented on pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
SteNicholas commented on pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#issuecomment-1047510831


   @yaooqinn @pan3793, please help to merge.


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#issuecomment-1046278968


   # [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1938](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7ccf467) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/1d64bb279e3fdc2c7ea130d127f9ae9d8c435e67?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1d64bb2) will **decrease** coverage by `0.02%`.
   > The diff coverage is `94.11%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/graphs/tree.svg?width=650&height=150&src=pr&token=925D4tb9AH&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1938      +/-   ##
   ============================================
   - Coverage     60.96%   60.93%   -0.03%     
   + Complexity       72       69       -3     
   ============================================
     Files           304      302       -2     
     Lines         14819    14795      -24     
     Branches       1915     1914       -1     
   ============================================
   - Hits           9034     9015      -19     
   + Misses         5015     5009       -6     
   - Partials        770      771       +1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...ine/flink/operation/FlinkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRmxpbmtTUUxPcGVyYXRpb25NYW5hZ2VyLnNjYWxh) | `85.29% <66.66%> (-2.21%)` | :arrow_down: |
   | [...uubi/engine/flink/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `81.81% <100.00%> (+0.73%)` | :arrow_up: |
   | [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `96.32% <100.00%> (+0.02%)` | :arrow_up: |
   | [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `92.50% <0.00%> (-2.50%)` | :arrow_down: |
   | [...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvY0J1aWxkZXIuc2NhbGE=) | `83.48% <0.00%> (-0.92%)` | :arrow_down: |
   | [...yuubi/sql/zorder/OptimizeZorderStatementBase.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstY29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC96b3JkZXIvT3B0aW1pemVab3JkZXJTdGF0ZW1lbnRCYXNlLnNjYWxh) | `71.42% <0.00%> (ø)` | |
   | [...ql/sqlclassification/KyuubiSqlClassification.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9zcWxjbGFzc2lmaWNhdGlvbi9LeXV1YmlTcWxDbGFzc2lmaWNhdGlvbi5zY2FsYQ==) | | |
   | [...g/apache/kyuubi/sql/RepartitionBeforeWriting.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9SZXBhcnRpdGlvbkJlZm9yZVdyaXRpbmcuc2NhbGE=) | | |
   | [...sqlclassification/KyuubiGetSqlClassification.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9zcWxjbGFzc2lmaWNhdGlvbi9LeXV1YmlHZXRTcWxDbGFzc2lmaWNhdGlvbi5zY2FsYQ==) | | |
   | [...org/apache/kyuubi/sql/RebalanceBeforeWriting.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0yL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9SZWJhbGFuY2VCZWZvcmVXcml0aW5nLnNjYWxh) | `80.00% <0.00%> (ø)` | |
   | ... and [2 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [1d64bb2...7ccf467](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] link3280 commented on a change in pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
link3280 commented on a change in pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#discussion_r811152718



##########
File path: externals/kyuubi-flink-sql-engine/src/test/scala/org/apache/kyuubi/engine/flink/operation/FlinkOperationSuite.scala
##########
@@ -761,4 +761,16 @@ class FlinkOperationSuite extends WithFlinkSQLEngine with HiveJDBCTestHelper {
         .getStringVal.getValues.get(0) === "tmp.hello")
     }
   }
+
+  test("ensure result max rows") {

Review comment:
       @SteNicholas Sure. Thanks for pointing that out.




-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] yaooqinn closed pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
yaooqinn closed pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938


   


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#issuecomment-1046278968


   # [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1938](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (80020ce) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/f25e5c93b8848524927f8e0c08e3d533a745a580?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f25e5c9) will **decrease** coverage by `0.00%`.
   > The diff coverage is `94.44%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/graphs/tree.svg?width=650&height=150&src=pr&token=925D4tb9AH&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1938      +/-   ##
   ============================================
   - Coverage     60.89%   60.88%   -0.01%     
     Complexity       69       69              
   ============================================
     Files           302      302              
     Lines         14797    14809      +12     
     Branches       1915     1915              
   ============================================
   + Hits           9010     9016       +6     
   - Misses         5020     5022       +2     
   - Partials        767      771       +4     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...ine/flink/operation/FlinkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRmxpbmtTUUxPcGVyYXRpb25NYW5hZ2VyLnNjYWxh) | `86.11% <80.00%> (-1.39%)` | :arrow_down: |
   | [...uubi/engine/flink/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `81.81% <100.00%> (+0.73%)` | :arrow_up: |
   | [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `96.32% <100.00%> (+0.02%)` | :arrow_up: |
   | [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `92.50% <0.00%> (-2.50%)` | :arrow_down: |
   | [...rg/apache/kyuubi/engine/trino/TrinoStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL1RyaW5vU3RhdGVtZW50LnNjYWxh) | `67.46% <0.00%> (-2.41%)` | :arrow_down: |
   | [...n/scala/org/apache/kyuubi/KyuubiSQLException.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9LeXV1YmlTUUxFeGNlcHRpb24uc2NhbGE=) | `92.77% <0.00%> (-1.21%)` | :arrow_down: |
   | [...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvY0J1aWxkZXIuc2NhbGE=) | `83.48% <0.00%> (-0.92%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f25e5c9...80020ce](https://codecov.io/gh/apache/incubator-kyuubi/pull/1938?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] SteNicholas commented on a change in pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#discussion_r810715345



##########
File path: externals/kyuubi-flink-sql-engine/src/test/scala/org/apache/kyuubi/engine/flink/operation/FlinkOperationSuite.scala
##########
@@ -761,4 +761,16 @@ class FlinkOperationSuite extends WithFlinkSQLEngine with HiveJDBCTestHelper {
         .getStringVal.getValues.get(0) === "tmp.hello")
     }
   }
+
+  test("ensure result max rows") {

Review comment:
       Could you please add the test case for setting the `ENGINE_FLINK_MAX_ROWS`?

##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala
##########
@@ -1199,4 +1199,13 @@ object KyuubiConf {
       .version("1.5.0")
       .stringConf
       .createOptional
+
+  val ENGINE_FLINK_MAX_ROWS: ConfigEntry[Int] =
+    buildConf("engine.flink.max.rows")
+      .doc("Max rows of Flink query results. For batch queries, rows that exceeds the limit " +
+        "would be ignored. For streaming queries, the query would be canceled if the limit " +
+        "is reached.")
+      .version("1.5.0")
+      .intConf
+      .createWithDefault(500)

Review comment:
       IMO, this could refer to the default value of the `SqlClientOptions#EXECUTION_MAX_TABLE_RESULT_ROWS` option.

##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala
##########
@@ -1199,4 +1199,13 @@ object KyuubiConf {
       .version("1.5.0")
       .stringConf
       .createOptional
+
+  val ENGINE_FLINK_MAX_ROWS: ConfigEntry[Int] =
+    buildConf("engine.flink.max.rows")

Review comment:
       ```suggestion
       buildConf("session.engine.flink.max.rows")
   ```




-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] yaooqinn closed pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
yaooqinn closed pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938


   


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] SteNicholas removed a comment on pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
SteNicholas removed a comment on pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#issuecomment-1047510831


   @yaooqinn @pan3793, please help to merge.


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] yaooqinn commented on pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#issuecomment-1047558113


   thanks @link3280 @SteNicholas for the work, merged to master


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] link3280 commented on a change in pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
link3280 commented on a change in pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#discussion_r811153680



##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala
##########
@@ -1199,4 +1199,13 @@ object KyuubiConf {
       .version("1.5.0")
       .stringConf
       .createOptional
+
+  val ENGINE_FLINK_MAX_ROWS: ConfigEntry[Int] =
+    buildConf("engine.flink.max.rows")

Review comment:
       OK. But I'm wondering if we should add `result` to make it `session.engine.flink.result.max.rows`?




-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] link3280 commented on a change in pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
link3280 commented on a change in pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#discussion_r811153680



##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala
##########
@@ -1199,4 +1199,13 @@ object KyuubiConf {
       .version("1.5.0")
       .stringConf
       .createOptional
+
+  val ENGINE_FLINK_MAX_ROWS: ConfigEntry[Int] =
+    buildConf("engine.flink.max.rows")

Review comment:
       OK. And I'm wondering if we should add `result` to make it `session.engine.flink.result.max.rows`?




-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] SteNicholas commented on pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
SteNicholas commented on pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#issuecomment-1046369932


   @link3280, thanks for your contribution. I left certain comments for this pull request.


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] SteNicholas commented on a change in pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#discussion_r811585775



##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala
##########
@@ -132,12 +133,16 @@ class ExecuteStatement(
       while (loop) {
         Thread.sleep(50) // slow the processing down
 
-        val result = executor.snapshotResult(sessionId, resultId, 2)
+        val pageSize = Math.min(500, resultMaxRows)
+        val result = executor.snapshotResult(sessionId, resultId, pageSize)
         result.getType match {
           case TypedResult.ResultType.PAYLOAD =>
-            rows.clear()
             (1 to result.getPayload).foreach { page =>
-              rows ++= executor.retrieveResultPage(resultId, page).asScala
+              if (rows.size < resultMaxRows) {

Review comment:
       ```suggestion
                 if (rows.size <= resultMaxRows) {
   ```




-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] yanghua commented on a change in pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#discussion_r811216991



##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala
##########
@@ -1199,4 +1199,13 @@ object KyuubiConf {
       .version("1.5.0")
       .stringConf
       .createOptional
+
+  val ENGINE_FLINK_MAX_ROWS: ConfigEntry[Int] =
+    buildConf("engine.flink.max.rows")

Review comment:
       > OK. And I'm wondering if we should add `result` to make it `session.engine.flink.result.max.rows`?
   
   maybe, `result` is not necessary, it's too long if we add 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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] link3280 commented on a change in pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
link3280 commented on a change in pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#discussion_r811603425



##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala
##########
@@ -132,12 +133,16 @@ class ExecuteStatement(
       while (loop) {
         Thread.sleep(50) // slow the processing down
 
-        val result = executor.snapshotResult(sessionId, resultId, 2)
+        val pageSize = Math.min(500, resultMaxRows)
+        val result = executor.snapshotResult(sessionId, resultId, pageSize)
         result.getType match {
           case TypedResult.ResultType.PAYLOAD =>
-            rows.clear()
             (1 to result.getPayload).foreach { page =>
-              rows ++= executor.retrieveResultPage(resultId, page).asScala
+              if (rows.size < resultMaxRows) {

Review comment:
       @SteNicholas Why pull another page if we already have the max number of rows?




-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] SteNicholas commented on pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
SteNicholas commented on pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#issuecomment-1047466489


   @link3280, the CI is failed. Please check the CI.


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] yanghua commented on a change in pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#discussion_r811026231



##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala
##########
@@ -1199,4 +1199,13 @@ object KyuubiConf {
       .version("1.5.0")
       .stringConf
       .createOptional
+
+  val ENGINE_FLINK_MAX_ROWS: ConfigEntry[Int] =
+    buildConf("engine.flink.max.rows")

Review comment:
       +1 for adding "session." prefix




-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] link3280 commented on pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
link3280 commented on pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#issuecomment-1046159849


   cc @pan3793 @yanghua @yaooqinn @SteNicholas 


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] SteNicholas removed a comment on pull request #1938: [KYUUBI #1883] Support max result rows for Flink queries

Posted by GitBox <gi...@apache.org>.
SteNicholas removed a comment on pull request #1938:
URL: https://github.com/apache/incubator-kyuubi/pull/1938#issuecomment-1047466489


   @link3280, the CI is failed. Please check the CI.


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org