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 2021/09/12 06:21:36 UTC
[GitHub] [incubator-kyuubi] i7xh opened a new pull request #1085: Add MaxOutputRows rule for output rows limitation to avoid huge output unexpectly
i7xh opened a new pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085
Add MaxOutputRows rule for output rows limitation to avoid huge output unexpectedly
<!--
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
<img width="1440" alt="截屏2021-09-12 下午12 19 28" src="https://user-images.githubusercontent.com/635169/132972063-b12937bb-807a-47bd-8d21-835d83031191.png">
'[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
We support the PR feature with limitation that avoid huge output rows in user ad-hoc query unexpected，mainly used in below cases:
- CASE 1:
```
INSERT OVERWRITE DIRECTORY (path=STRING)?
USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
SELECT ...
```
- CASE 2:
```
SELECT [c1, c2, ...]
```
- CASE 3:
```
WITH CTE AS (...)
SELECT [c1, c2, ...] FROM Express(CTE) ...
```
### _How was this patch tested?_
- [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [x] Add screenshots for manual tests if appropriate
<img width="1440" alt="截屏2021-09-12 下午12 19 28" src="https://user-images.githubusercontent.com/635169/132972078-c4821135-0520-420d-9ab8-24e124f6c6c9.png">
- [x] [Run test](https://kyuubi.readthedocs.io/en/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] codecov-commenter commented on pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#issuecomment-919133338
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 [#1085](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (14a73c4) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/aa1a7d63beef1c4d5dbe39f5baab592955686bd7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (aa1a7d6) will **decrease** coverage by `0.04%`.
> The diff coverage is `60.00%`.
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 #1085 +/- ##
============================================
- Coverage 79.44% 79.39% -0.05%
- Complexity 87 94 +7
============================================
Files 176 177 +1
Lines 6552 6572 +20
Branches 775 782 +7
============================================
+ Hits 5205 5218 +13
- Misses 897 899 +2
- Partials 450 455 +5
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC93YXRjaGRvZy9Gb3JjZWRNYXhPdXRwdXRSb3dzUnVsZS5zY2FsYQ==) | `46.66% <46.66%> (ø)` | |
| [...in/scala/org/apache/kyuubi/sql/KyuubiSQLConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTUUxDb25mLnNjYWxh) | `100.00% <100.00%> (ø)` | |
| [...rg/apache/kyuubi/sql/KyuubiSparkSQLExtension.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTcGFya1NRTEV4dGVuc2lvbi5zY2FsYQ==) | `92.30% <100.00%> (+0.64%)` | :arrow_up: |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `58.69% <0.00%> (+2.17%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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/1085?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 [aa1a7d6...14a73c4](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r708114697
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_MAX_OUTPUTROWS) match {
+ case Some(maxOutputRows) => plan match {
Review comment:
About this issue
1. process `plan.maxRows is none` that node without limit statement
2. `plan.maxRows.get > maxOutputRows` illustrate exists limit statement in query, so don't cover user choice, or else user query result exceed expectation as a bug, so skip the situation
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r707946937
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
+ case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)
+ case Origin(Some(_), Some(0)) => GlobalLimit(forcedMaxOutputRows, project)
Review comment:
> why we need match `Origin` if the plan is resolved ?
1. It can actually remove `case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)` before plan is resolved
2. Match `Origin` mainly applied to `SELECT ...` or `WITH CTE(...) SELECT...`, output nodes such as `Project` always starts 0 position on Analyzer，so it judge the output nodes depend on `Origin(_, Some(0))`
--
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 #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#issuecomment-919133338
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 [#1085](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (14a73c4) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/5602c29b3694b3d2fdec6ab8334687180d9135d9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5602c29) will **increase** coverage by `0.21%`.
> The diff coverage is `60.00%`.
> :exclamation: Current head 14a73c4 differs from pull request most recent head c1a88e5. Consider uploading reports for the commit c1a88e5 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 #1085 +/- ##
============================================
+ Coverage 79.18% 79.39% +0.21%
- Complexity 90 94 +4
============================================
Files 177 177
Lines 6620 6572 -48
Branches 783 782 -1
============================================
- Hits 5242 5218 -24
+ Misses 924 899 -25
- Partials 454 455 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC93YXRjaGRvZy9Gb3JjZWRNYXhPdXRwdXRSb3dzUnVsZS5zY2FsYQ==) | `46.66% <46.66%> (ø)` | |
| [...in/scala/org/apache/kyuubi/sql/KyuubiSQLConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTUUxDb25mLnNjYWxh) | `100.00% <100.00%> (ø)` | |
| [...rg/apache/kyuubi/sql/KyuubiSparkSQLExtension.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTcGFya1NRTEV4dGVuc2lvbi5zY2FsYQ==) | `92.30% <100.00%> (+0.64%)` | :arrow_up: |
| [.../org/apache/kyuubi/operation/KyuubiOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uLnNjYWxh) | `37.50% <0.00%> (-12.50%)` | :arrow_down: |
| [...in/scala/org/apache/kyuubi/sql/zorder/Zorder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC96b3JkZXIvWm9yZGVyLnNjYWxh) | `82.35% <0.00%> (-4.83%)` | :arrow_down: |
| [...kyuubi/engine/spark/operation/SparkOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vU3BhcmtPcGVyYXRpb24uc2NhbGE=) | `69.35% <0.00%> (-0.49%)` | :arrow_down: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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==) | `95.30% <0.00%> (-0.09%)` | :arrow_down: |
| [...ubi/engine/spark/operation/PlanOnlyStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vUGxhbk9ubHlTdGF0ZW1lbnQuc2NhbGE=) | | |
| [...uubi/engine/spark/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `89.02% <0.00%> (+0.13%)` | :arrow_up: |
| ... and [3 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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/1085?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/1085?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 [5602c29...c1a88e5](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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] ulysses-you commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r710837568
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_MAX_OUTPUTROWS) match {
+ case Some(maxOutputRows) => plan match {
Review comment:
Do you mean if user's query limit 100 and the config is 10 then we can't add the limit ? I think we should add limit as long as `plan.maxRows is none` or `plan.maxRows.get > maxOutputRows`. Otherwise it's hard to say which limit is added by us.
--
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] ulysses-you commented on pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#issuecomment-922177845
thanks, 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] codecov-commenter edited a comment on pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#issuecomment-919133338
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 [#1085](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (939d255) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/7da30bd9921dd7957422eab443388335fe809822?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7da30bd) will **increase** coverage by `0.01%`.
> The diff coverage is `72.22%`.
> :exclamation: Current head 939d255 differs from pull request most recent head 77aa6ff. Consider uploading reports for the commit 77aa6ff to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 #1085 +/- ##
============================================
+ Coverage 79.19% 79.21% +0.01%
- Complexity 90 101 +11
============================================
Files 177 178 +1
Lines 6620 6638 +18
Branches 783 789 +6
============================================
+ Hits 5243 5258 +15
+ Misses 923 922 -1
- Partials 454 458 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC93YXRjaGRvZy9Gb3JjZWRNYXhPdXRwdXRSb3dzUnVsZS5zY2FsYQ==) | `61.53% <61.53%> (ø)` | |
| [...in/scala/org/apache/kyuubi/sql/KyuubiSQLConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTUUxDb25mLnNjYWxh) | `100.00% <100.00%> (ø)` | |
| [...rg/apache/kyuubi/sql/KyuubiSparkSQLExtension.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTcGFya1NRTEV4dGVuc2lvbi5zY2FsYQ==) | `92.30% <100.00%> (+0.64%)` | :arrow_up: |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `56.52% <0.00%> (-2.18%)` | :arrow_down: |
| [.../org/apache/kyuubi/operation/KyuubiOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uLnNjYWxh) | `55.35% <0.00%> (+5.35%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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/1085?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 [7da30bd...77aa6ff](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#issuecomment-919133338
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 [#1085](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (142561b) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/5602c29b3694b3d2fdec6ab8334687180d9135d9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5602c29) will **decrease** coverage by `0.02%`.
> The diff coverage is `87.50%`.
> :exclamation: Current head 142561b differs from pull request most recent head a1101c1. Consider uploading reports for the commit a1101c1 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 #1085 +/- ##
============================================
- Coverage 79.24% 79.21% -0.03%
- Complexity 90 97 +7
============================================
Files 177 178 +1
Lines 6620 6636 +16
Branches 783 788 +5
============================================
+ Hits 5246 5257 +11
- Misses 919 925 +6
+ Partials 455 454 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC93YXRjaGRvZy9Gb3JjZWRNYXhPdXRwdXRSb3dzUnVsZS5zY2FsYQ==) | `81.81% <81.81%> (ø)` | |
| [...in/scala/org/apache/kyuubi/sql/KyuubiSQLConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTUUxDb25mLnNjYWxh) | `100.00% <100.00%> (ø)` | |
| [...rg/apache/kyuubi/sql/KyuubiSparkSQLExtension.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTcGFya1NRTEV4dGVuc2lvbi5zY2FsYQ==) | `92.30% <100.00%> (+0.64%)` | :arrow_up: |
| [.../org/apache/kyuubi/operation/KyuubiOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uLnNjYWxh) | `50.00% <0.00%> (-5.36%)` | :arrow_down: |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `56.52% <0.00%> (-2.18%)` | :arrow_down: |
| [...pache/kyuubi/sql/KyuubiQueryStagePreparation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlRdWVyeVN0YWdlUHJlcGFyYXRpb24uc2NhbGE=) | `81.37% <0.00%> (+0.98%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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/1085?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 [5602c29...a1101c1](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r707298445
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
Review comment:
The rule will insert GlobalLimit node in tree nodes, it changed original logicalPlan target, it make difference target between Analyzer and Optimizer, yet They should be idempotent
--
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] ulysses-you commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r708783362
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
Review comment:
also cc @turboFei for the use case:
> In our scenarios, ad-hoc query result save as hdfs files with INSERT OVERWRITE DIRECTORY(case 1) statement，SELECT ... or WITH CTE ... will lead to save as redis，but it's conscious InsertIntoHiveTable for user to produce the data as they want, so it's no need to limit InsertIntoHiveTable output
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r707929251
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
Review comment:
> why we need handle `InsertIntoDataSourceDirCommand` specially ? Seems this PR is only for query and not include the writing command (e.g. `InsertIntoHiveTable` etc)
In our scenarios, ad-hoc query result save as hdfs files with `INSERT OVERWRITE DIRECTORY`(case 1) statement，`SELECT ...` or `WITH CTE ...` will lead to save as redis，but it's conscious InsertIntoHiveTable for user to produce the data as they want
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r711489594
##########
File path: dev/kyuubi-extension-spark-3-1/src/test/scala/org/apache/spark/sql/KyuubiExtensionSuite.scala
##########
@@ -1303,4 +1303,96 @@ class KyuubiExtensionSuite extends KyuubiSparkSQLExtensionTest {
}
}
}
+
+ test("test watchdog with query forceMaxOutputRows") {
+
+ withSQLConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS.key -> "10") {
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 1")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(1))
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 11")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(10))
+
+ assert(!sql("SELECT count(*) FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql(
+ """
+ |SELECT c1, COUNT(*)
+ |FROM VALUES(1, 'a'), (2, 'b') AS t(c1, c2)
+ |GROUP BY c1
+ |""".stripMargin).queryExecution.analyzed.isInstanceOf[GlobalLimit])
Review comment:
> This query used the plan `LocalRelation` and after 3.2.0 the `LocalRelation`'s maxRows will be the data size. That said, if you have a query `SELECT * FROM VALUES(1, 'a'), (2, 'b') AS t(c1, c2)`, the root Project node maxRows will be `Some(2)` instead of `None`. Then the test will fail if we switch the Spark version from 3.1 to 3.2.
Clear! Thanks for your advice!
--
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] ulysses-you closed pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you closed pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r708105163
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
+ case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)
+ case Origin(Some(_), Some(0)) => GlobalLimit(forcedMaxOutputRows, project)
Review comment:
SQL trees may contain other node of same type, especially there may have `Project` or `Aggregate` in sub-query
It's critical to find the root output node such as `Project` or `Aggregate`, so root `Project match Origin(Some(_), Some(0)` and `Aggregate match Origin(None, None)`
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r708035950
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
Review comment:
> It's quite customized to match `InsertIntoDataSourceDirCommand, let's match query only.
>
> And for your use case, we can apply this limit rule first then you can apply some other cutom rule (e.g. add `InsertIntoDataSourceDirCommand`).
OK，So I remove `InsertIntoDataSourceDirCommand` node process from the code，reserve pure query only
--
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] ulysses-you commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r711423244
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{Aggregate, Limit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation
+* to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+*
+* case 1:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 2:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+
+ private def canInsertLimit(p: LogicalPlan): Boolean = {
Review comment:
can we put it as parameter `maxOutputRows: Option[Int]` ?
##########
File path: dev/kyuubi-extension-spark-3-1/src/test/scala/org/apache/spark/sql/KyuubiExtensionSuite.scala
##########
@@ -1303,4 +1303,96 @@ class KyuubiExtensionSuite extends KyuubiSparkSQLExtensionTest {
}
}
}
+
+ test("test watchdog with query forceMaxOutputRows") {
+
+ withSQLConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS.key -> "10") {
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 1")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(1))
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 11")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(10))
+
+ assert(!sql("SELECT count(*) FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql(
+ """
+ |SELECT c1, COUNT(*)
+ |FROM VALUES(1, 'a'), (2, 'b') AS t(c1, c2)
+ |GROUP BY c1
+ |""".stripMargin).queryExecution.analyzed.isInstanceOf[GlobalLimit])
Review comment:
It's flaky with Spark3.2.0 that the issue of some logical plan has not overridden maxRows is fixed. see https://github.com/apache/spark/pull/30443.
We can use the test table, see `KyuubiExtensionSuite.setupData`.
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r710855635
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_MAX_OUTPUTROWS) match {
+ case Some(maxOutputRows) => plan match {
Review comment:
> Do you mean if user's query limit 100 and the config is 10 then we can't add the limit ?
Yes.
> I think we should add limit as long as `plan.maxRows is none` or `plan.maxRows.get > maxOutputRows`. Otherwise it's hard to say which limit is added by us.
Do you mean using maxOutputRows cover user limit setting ？
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r707298445
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
Review comment:
The rule will insert GlobalLimit node in tree nodes, it changed original logicalPlan target, it make difference target between Analyzer and Optimizer, yet It should be idempotent
--
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] i7xh commented on pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#issuecomment-918999187
I have already support `Aggregate` process
--
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] ulysses-you commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r708033062
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
+ case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)
+ case Origin(Some(_), Some(0)) => GlobalLimit(forcedMaxOutputRows, project)
Review comment:
what's difference if we only match `Project` ? do you mean we will get a wrong node to add limit ?
--
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] ulysses-you commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r711003304
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{Aggregate, GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation
+* to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+*
+* case 1:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 2:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* case 3 (customize yourself):
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan.maxRows match {
Review comment:
can we format the code like this:
```
private def canInsertLimit(p: LogicalPlan): Boolean = {
val supported = p match {
case _: Project => true
case _: Aggregate => true
case _: Filter => true
case _: Sort => true
case _: Union => true
case _: Join => true
case _: Window => true
case _: Generate => true
case Limit(_, _) => true
case _: Repartition => true
case _: RepartitionByExpression => true
case _ => false
}
supported && !p.maxRows.exists(_ <= forcedMaxOutputRows)
}
plan match {
case p if canInsertLimit(p) => Limit(Literal(forcedMaxOutputRows), p)
case _ => plan
}
```
--
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] ulysses-you commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r707894555
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_MAX_OUTPUTROWS) match {
+ case Some(maxOutputRows) => plan match {
Review comment:
we can add limit if:
- plan.maxRows is none
- plan.maxRows.get > maxOutputRows
--
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] ulysses-you commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r711003821
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{Aggregate, GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation
+* to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+*
+* case 1:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 2:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* case 3 (customize yourself):
Review comment:
this docs is outdate
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r711451568
##########
File path: dev/kyuubi-extension-spark-3-1/src/test/scala/org/apache/spark/sql/KyuubiExtensionSuite.scala
##########
@@ -1303,4 +1303,96 @@ class KyuubiExtensionSuite extends KyuubiSparkSQLExtensionTest {
}
}
}
+
+ test("test watchdog with query forceMaxOutputRows") {
+
+ withSQLConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS.key -> "10") {
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 1")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(1))
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 11")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(10))
+
+ assert(!sql("SELECT count(*) FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql(
+ """
+ |SELECT c1, COUNT(*)
+ |FROM VALUES(1, 'a'), (2, 'b') AS t(c1, c2)
+ |GROUP BY c1
+ |""".stripMargin).queryExecution.analyzed.isInstanceOf[GlobalLimit])
Review comment:
> It's flaky with Spark3.2.0 that the issue of some logical plan has not overridden maxRows is fixed. see [apache/spark#30443](https://github.com/apache/spark/pull/30443).
>
> We can use the test table, see `KyuubiExtensionSuite.setupData`.
_见贤思齐_
I read the [apache/spark#30443](https://github.com/apache/spark/pull/30443). It seems to get more exactly maxRows in Optimizer? I don't understand how affect the test case changed, as `VALUES(1, 'a'), (2, 'b') AS t(c1, c2)`?
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r708107709
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
Review comment:
> we can match more nodes e.g. (Filter, Aggregate, Join ...), how about:
>
> ```
> case _: ExecutedCommandExec => plan
> case _: CommandResultExec => plan
> case _: V2CommandExec => plan
> case c: DataWritingCommandExec => c.copy(child = apply(c.child))
> case p => apply(p)
> ```
Generally root output node of a query is `Project` or `Aggregate` in Analyzer，tree node level or order will be changed in Optimizer
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r711452193
##########
File path: dev/kyuubi-extension-spark-3-1/src/test/scala/org/apache/spark/sql/KyuubiExtensionSuite.scala
##########
@@ -1303,4 +1303,96 @@ class KyuubiExtensionSuite extends KyuubiSparkSQLExtensionTest {
}
}
}
+
+ test("test watchdog with query forceMaxOutputRows") {
+
+ withSQLConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS.key -> "10") {
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 1")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(1))
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 11")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(10))
+
+ assert(!sql("SELECT count(*) FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql(
+ """
+ |SELECT c1, COUNT(*)
+ |FROM VALUES(1, 'a'), (2, 'b') AS t(c1, c2)
+ |GROUP BY c1
+ |""".stripMargin).queryExecution.analyzed.isInstanceOf[GlobalLimit])
Review comment:
I have replaced it with setupData
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r708107709
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
Review comment:
> we can match more nodes e.g. (Filter, Aggregate, Join ...), how about:
>
> ```
> case _: ExecutedCommandExec => plan
> case _: CommandResultExec => plan
> case _: V2CommandExec => plan
> case c: DataWritingCommandExec => c.copy(child = apply(c.child))
> case p => apply(p)
> ```
Generally root output node is `Project` or `Aggregate` in Analyzer，tree node level or order will be changed in Optimizer
--
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] ulysses-you commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r710876620
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_MAX_OUTPUTROWS) match {
+ case Some(maxOutputRows) => plan match {
Review comment:
yes, otherwise this config has no meaning
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r711451568
##########
File path: dev/kyuubi-extension-spark-3-1/src/test/scala/org/apache/spark/sql/KyuubiExtensionSuite.scala
##########
@@ -1303,4 +1303,96 @@ class KyuubiExtensionSuite extends KyuubiSparkSQLExtensionTest {
}
}
}
+
+ test("test watchdog with query forceMaxOutputRows") {
+
+ withSQLConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS.key -> "10") {
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 1")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(1))
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 11")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(10))
+
+ assert(!sql("SELECT count(*) FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql(
+ """
+ |SELECT c1, COUNT(*)
+ |FROM VALUES(1, 'a'), (2, 'b') AS t(c1, c2)
+ |GROUP BY c1
+ |""".stripMargin).queryExecution.analyzed.isInstanceOf[GlobalLimit])
Review comment:
> It's flaky with Spark3.2.0 that the issue of some logical plan has not overridden maxRows is fixed. see [apache/spark#30443](https://github.com/apache/spark/pull/30443).
>
> We can use the test table, see `KyuubiExtensionSuite.setupData`.
_见贤思齐_
I read the [apache/spark#30443](https://github.com/apache/spark/pull/30443). It seems to get more exactly maxRows in Optimizer? I don't understand how affect the test case changed, as `VALUES(1, 'a'), (2, 'b') AS t(c1, c2)`
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r711452357
##########
File path: dev/kyuubi-extension-spark-3-1/src/test/scala/org/apache/spark/sql/KyuubiExtensionSuite.scala
##########
@@ -1303,4 +1303,96 @@ class KyuubiExtensionSuite extends KyuubiSparkSQLExtensionTest {
}
}
}
+
+ test("test watchdog with query forceMaxOutputRows") {
+
+ withSQLConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS.key -> "10") {
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 1")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(1))
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 11")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(10))
+
+ assert(!sql("SELECT count(*) FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql(
+ """
+ |SELECT c1, COUNT(*)
+ |FROM VALUES(1, 'a'), (2, 'b') AS t(c1, c2)
+ |GROUP BY c1
+ |""".stripMargin).queryExecution.analyzed.isInstanceOf[GlobalLimit])
Review comment:
> > It's flaky with Spark3.2.0 that the issue of some logical plan has not overridden maxRows is fixed. see [apache/spark#30443](https://github.com/apache/spark/pull/30443).
> > We can use the test table, see `KyuubiExtensionSuite.setupData`.
>
> _见贤思齐_
> I read the [apache/spark#30443](https://github.com/apache/spark/pull/30443). It seems to get more exactly maxRows in Optimizer? I don't understand how affect the test case changed yet, as `VALUES(1, 'a'), (2, 'b') AS t(c1, c2)`?
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{Aggregate, Limit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation
+* to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+*
+* case 1:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 2:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+
+ private def canInsertLimit(p: LogicalPlan): Boolean = {
Review comment:
> can we put it as parameter `maxOutputRows: Option[Int]` ?
OK, I unified the maxOutputRows quote
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r710225261
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/KyuubiSQLConf.scala
##########
@@ -94,4 +94,13 @@ object KyuubiSQLConf {
.version("1.4.0")
.intConf
.createOptional
+
+ val WATCHDOG_FORCED_MAXOUTPUTROWS =
+ buildConf("spark.sql.watchdog.forcedMaxOutputRows")
+ .doc("Add MaxOutputRows rule for output rows limitation " +
+ "to avoid huge output rows of non-limit query unexpectedly, " +
+ "it's a optional, it's optional that works with defined")
Review comment:
> Words here seem to need a little improvement.
Fixed：https://github.com/apache/incubator-kyuubi/pull/1085/commits/c1a88e54cdb83f77dde8b820a4d3ee6688e86cff
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r710191442
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
+ case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)
+ case Origin(Some(_), Some(0)) => GlobalLimit(forcedMaxOutputRows, project)
Review comment:
> is there any issue if we also add limit for suquery ? I think we don't need care subquery as following:
>
> * scalar subquery; it return at most one row or exception
> * in subquery; it's ok to add limit so we can protect engine's driver (OOM)
> * exist subquery; it will be rewrite to scalar subquery, so it's also safe.
Add limit for subquery may make difference with origin query，my implement does not limit node to subquery
Do you tend to add subquery with above three reasons or not？
--
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] ulysses-you commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r711470504
##########
File path: dev/kyuubi-extension-spark-3-1/src/test/scala/org/apache/spark/sql/KyuubiExtensionSuite.scala
##########
@@ -1303,4 +1303,96 @@ class KyuubiExtensionSuite extends KyuubiSparkSQLExtensionTest {
}
}
}
+
+ test("test watchdog with query forceMaxOutputRows") {
+
+ withSQLConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS.key -> "10") {
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 1")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(1))
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 11")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(10))
+
+ assert(!sql("SELECT count(*) FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql(
+ """
+ |SELECT c1, COUNT(*)
+ |FROM VALUES(1, 'a'), (2, 'b') AS t(c1, c2)
+ |GROUP BY c1
+ |""".stripMargin).queryExecution.analyzed.isInstanceOf[GlobalLimit])
Review comment:
This query used the plan `LocalRelation` and after 3.2.0 the `LocalRelation`'s maxRows will be the data size. That said, if you have a query `SELECT * FROM VALUES(1, 'a'), (2, 'b') AS t(c1, c2)`, the root Project node maxRows will be `Some(2)` instead of `None`. Then the test will fail if we switch the Spark version from 3.1 to 3.2.
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r707288843
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_MAX_OUTPUTROWS) match {
+ case Some(maxOutputRows) => plan match {
Review comment:
It's origin limit the non-limit query, but It cann't cover with existed limit statement, It's easy to misunderstanding the option name，I rename `spark.sql.watchdog.maxOutputRows` to `spark.sql.watchdog.forcedMaxOutputRows`
--
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 #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#issuecomment-919133338
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 [#1085](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (13734f8) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/5602c29b3694b3d2fdec6ab8334687180d9135d9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5602c29) will **decrease** coverage by `0.10%`.
> The diff coverage is `60.00%`.
> :exclamation: Current head 13734f8 differs from pull request most recent head d3c02db. Consider uploading reports for the commit d3c02db to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 #1085 +/- ##
============================================
- Coverage 79.24% 79.14% -0.11%
- Complexity 90 97 +7
============================================
Files 177 178 +1
Lines 6620 6640 +20
Branches 783 790 +7
============================================
+ Hits 5246 5255 +9
- Misses 919 926 +7
- Partials 455 459 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC93YXRjaGRvZy9Gb3JjZWRNYXhPdXRwdXRSb3dzUnVsZS5zY2FsYQ==) | `46.66% <46.66%> (ø)` | |
| [...in/scala/org/apache/kyuubi/sql/KyuubiSQLConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTUUxDb25mLnNjYWxh) | `100.00% <100.00%> (ø)` | |
| [...rg/apache/kyuubi/sql/KyuubiSparkSQLExtension.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTcGFya1NRTEV4dGVuc2lvbi5zY2FsYQ==) | `92.30% <100.00%> (+0.64%)` | :arrow_up: |
| [.../org/apache/kyuubi/operation/KyuubiOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uLnNjYWxh) | `50.00% <0.00%> (-5.36%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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/1085?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 [5602c29...d3c02db](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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] ulysses-you commented on a change in pull request #1085: Add maxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r706986848
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
Review comment:
it's better to inject this rule into optimizer: `injectOptimizerRule` then we don't need this check
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_MAX_OUTPUTROWS) match {
+ case Some(maxOutputRows) => plan match {
Review comment:
how about match `plan.maxRows > maxOutputRows` ?
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r707929251
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
Review comment:
> why we need handle `InsertIntoDataSourceDirCommand` specially ? Seems this PR is only for query and not include the writing command (e.g. `InsertIntoHiveTable` etc)
In our scenarios, ad-hoc query result save as hdfs files with `INSERT OVERWRITE DIRECTORY`(case 1) statement，`SELECT ...` or `WITH CTE ...` will lead to save as redis，but it's conscious `InsertIntoHiveTable` for user to produce the data as they want, so it's no need to limit `InsertIntoHiveTable` output
--
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] zhouyifan279 commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
zhouyifan279 commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r708166999
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/KyuubiSQLConf.scala
##########
@@ -94,4 +94,13 @@ object KyuubiSQLConf {
.version("1.4.0")
.intConf
.createOptional
+
+ val WATCHDOG_FORCED_MAXOUTPUTROWS =
+ buildConf("spark.sql.watchdog.forcedMaxOutputRows")
+ .doc("Add MaxOutputRows rule for output rows limitation " +
+ "to avoid huge output rows of non-limit query unexpectedly, " +
+ "it's a optional, it's optional that works with defined")
Review comment:
Words here seem to need a little improvement.
--
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] ulysses-you commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r708011580
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
Review comment:
It's quite customized to match `InsertIntoDataSourceDirCommand, let's match query only.
And for your use case, we can apply this limit rule first then you can apply some other cutom rule (e.g. add `InsertIntoDataSourceDirCommand`).
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r707929251
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
Review comment:
> why we need handle `InsertIntoDataSourceDirCommand` specially ? Seems this PR is only for query and not include the writing command (e.g. `InsertIntoHiveTable` etc)
In our scenarios, ad-hoc query result save as hdfs files with `INSERT OVERWRITE DIRECTORY`(case 1) statement，`SELECT ...` or `WITH CTE ...` will lead to save as redis，but it's conscious `InsertIntoHiveTable` for user to produce the data as they want
--
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] ulysses-you commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r707892894
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
Review comment:
we can match more nodes e.g. (Filter, Aggregate, Join ...), how about:
```
case _: ExecutedCommandExec => plan
case _: CommandResultExec => plan
case _: V2CommandExec => plan
case c: DataWritingCommandExec => c.copy(child = apply(c.child))
case p => apply(p)
```
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
Review comment:
why we need handle `InsertIntoDataSourceDirCommand` specially ? Seems this PR is only for query and not include the writing command (e.g. `InsertIntoHiveTable` etc)
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
+ case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)
+ case Origin(Some(_), Some(0)) => GlobalLimit(forcedMaxOutputRows, project)
Review comment:
why we need match `Origin` if the plan is resolved ?
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r710855635
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_MAX_OUTPUTROWS) match {
+ case Some(maxOutputRows) => plan match {
Review comment:
> Do you mean if user's query limit 100 and the config is 10 then we can't add the limit ?
Yes.
> I think we should add limit as long as `plan.maxRows is none` or `plan.maxRows.get > maxOutputRows`. Otherwise it's hard to say which limit is added by us.
Do you mean using maxOutputRows as final limit cover user limit setting if plan.maxRows.get > maxOutputRows？
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r707880979
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_MAX_OUTPUTROWS) match {
+ case Some(maxOutputRows) => plan match {
Review comment:
> how about match `plan.maxRows > maxOutputRows` ?
if non-limit statement, plan.maxRows is None type，the rule will match non-limit statement and skip statement with limit
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r710855635
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_MAX_OUTPUTROWS) match {
+ case Some(maxOutputRows) => plan match {
Review comment:
> Do you mean if user's query limit 100 and the config is 10 then we can't add the limit ?
Yes.
> I think we should add limit as long as `plan.maxRows is none` or `plan.maxRows.get > maxOutputRows`. Otherwise it's hard to say which limit is added by us.
Do you mean using maxOutputRows as final limit cover user limit setting ？
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r710190711
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/KyuubiSQLConf.scala
##########
@@ -94,4 +94,13 @@ object KyuubiSQLConf {
.version("1.4.0")
.intConf
.createOptional
+
+ val WATCHDOG_FORCED_MAXOUTPUTROWS =
+ buildConf("spark.sql.watchdog.forcedMaxOutputRows")
+ .doc("Add MaxOutputRows rule for output rows limitation " +
+ "to avoid huge output rows of non-limit query unexpectedly, " +
+ "it's a optional, it's optional that works with defined")
Review comment:
Could you give me some suggestion?😂
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
+ case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)
+ case Origin(Some(_), Some(0)) => GlobalLimit(forcedMaxOutputRows, project)
Review comment:
> Seems a little hack, the `Origin` is used to distinguish if the plan is subquery ?
The `Origin` provides a location for TreeNodes in the context of their origin, it define as below(reference: https://github.com/apache/spark/blob/branch-3.1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala#L49
):
```scala
case class Origin(
line: Option[Int] = None,
startPosition: Option[Int] = None)
The chief output node as root with init position(None or Some(0)), it's not used to distinguish subquery, we can use subquery class to identify
> is there any issue if we also add limit for suquery ? I think we don't need care subquery as following:
>
> * scalar subquery; it return at most one row or exception
> * in subquery; it's ok to add limit so we can protect engine's driver (OOM)
> * exist subquery; it will be rewrite to scalar subquery, so it's also safe.
Add limit for subquery may make difference with origin query，my implement will not limit node to subquery
Do you tend to add subquery with above three reasons or not？
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r710728445
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
+ case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)
+ case Origin(Some(_), Some(0)) => GlobalLimit(forcedMaxOutputRows, project)
Review comment:
> > The Origin provides a location for TreeNodes in the context of their origin
> > it's not used to distinguish subquery, we can use subquery class to identify
>
> So I'm still not see why you need match origin here, the origin is used to help track error sql string. Is there any use case that Spark match the origin to do the simialr thing ?
Why I use origin to judge the output root node?
According SQL parse principle, it parse with top-down, the output root node is the top node, so I depend on `startPosition` to find root node, It works on sql parse with `antlr` , now the implement mainly use the `startPosition ` in `Token` parsed by antlr, `startPosition` notes that The index of the first character of this token relative to the beginning of the line at which it occurs, 0..n-1
As you said `Origin` is used to track sql string, it almost not match `Origin` in spark
--
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] ulysses-you commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r708243348
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
+ case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)
+ case Origin(Some(_), Some(0)) => GlobalLimit(forcedMaxOutputRows, project)
Review comment:
Seems a little hack, the `Origin` is used to distinguish if the plan is subquery ?
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r711008638
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{Aggregate, GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation
+* to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+*
+* case 1:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 2:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* case 3 (customize yourself):
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan.maxRows match {
Review comment:
OK, It's concise
--
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 #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#issuecomment-919133338
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 [#1085](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f35c471) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/7da30bd9921dd7957422eab443388335fe809822?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7da30bd) will **decrease** coverage by `0.06%`.
> The diff coverage is `59.09%`.
> :exclamation: Current head f35c471 differs from pull request most recent head 5b0383d. Consider uploading reports for the commit 5b0383d to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 #1085 +/- ##
============================================
- Coverage 79.19% 79.13% -0.07%
- Complexity 90 97 +7
============================================
Files 177 178 +1
Lines 6620 6642 +22
Branches 783 791 +8
============================================
+ Hits 5243 5256 +13
- Misses 923 927 +4
- Partials 454 459 +5
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC93YXRjaGRvZy9Gb3JjZWRNYXhPdXRwdXRSb3dzUnVsZS5zY2FsYQ==) | `47.05% <47.05%> (ø)` | |
| [...in/scala/org/apache/kyuubi/sql/KyuubiSQLConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTUUxDb25mLnNjYWxh) | `100.00% <100.00%> (ø)` | |
| [...rg/apache/kyuubi/sql/KyuubiSparkSQLExtension.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTcGFya1NRTEV4dGVuc2lvbi5zY2FsYQ==) | `92.30% <100.00%> (+0.64%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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/1085?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 [7da30bd...5b0383d](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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] ulysses-you commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r708781971
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
+ case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)
+ case Origin(Some(_), Some(0)) => GlobalLimit(forcedMaxOutputRows, project)
Review comment:
is there any issue if we also add limit for suquery ? I think we don't need care subquery as following:
- scalar subquery; it return at most one row or exception
- in subquery; it's ok to add limit so we can protect engine's driver (OOM)
- exist subquery; it will be rewrite to scalar subquery, so it's also safe.
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r711447138
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{Aggregate, Limit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation
+* to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+*
+* case 1:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 2:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+
+ private def canInsertLimit(p: LogicalPlan): Boolean = {
Review comment:
> can we put it as parameter `maxOutputRows: Option[Int]` ?
OK, I unified the maxOutputRows quote
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r710809753
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
+ case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)
+ case Origin(Some(_), Some(0)) => GlobalLimit(forcedMaxOutputRows, project)
Review comment:
I think out this issue, the plan run `match` operator on this rule , it's just the root node, so it doesn't travel matched plan children, thank your heuristic!
--
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 #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#issuecomment-919133338
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 [#1085](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (142561b) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/7da30bd9921dd7957422eab443388335fe809822?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7da30bd) will **increase** coverage by `0.02%`.
> The diff coverage is `87.50%`.
> :exclamation: Current head 142561b differs from pull request most recent head 5b0383d. Consider uploading reports for the commit 5b0383d to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 #1085 +/- ##
============================================
+ Coverage 79.19% 79.21% +0.02%
- Complexity 90 97 +7
============================================
Files 177 178 +1
Lines 6620 6636 +16
Branches 783 788 +5
============================================
+ Hits 5243 5257 +14
- Misses 923 925 +2
Partials 454 454
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC93YXRjaGRvZy9Gb3JjZWRNYXhPdXRwdXRSb3dzUnVsZS5zY2FsYQ==) | `81.81% <81.81%> (ø)` | |
| [...in/scala/org/apache/kyuubi/sql/KyuubiSQLConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTUUxDb25mLnNjYWxh) | `100.00% <100.00%> (ø)` | |
| [...rg/apache/kyuubi/sql/KyuubiSparkSQLExtension.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTcGFya1NRTEV4dGVuc2lvbi5zY2FsYQ==) | `92.30% <100.00%> (+0.64%)` | :arrow_up: |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `56.52% <0.00%> (-2.18%)` | :arrow_down: |
| [...pache/kyuubi/sql/KyuubiQueryStagePreparation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlRdWVyeVN0YWdlUHJlcGFyYXRpb24uc2NhbGE=) | `81.37% <0.00%> (+0.98%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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/1085?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 [7da30bd...5b0383d](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r707298445
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
Review comment:
the rule will insert GlobalLimit node in tree nodes, it changed original logicalPlan target, it make difference target between Analyzer and Optimizer, yet It should be idempotent
--
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 #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#issuecomment-919133338
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 [#1085](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77aa6ff) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/7da30bd9921dd7957422eab443388335fe809822?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7da30bd) will **decrease** coverage by `0.01%`.
> The diff coverage is `73.68%`.
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 #1085 +/- ##
============================================
- Coverage 79.19% 79.18% -0.02%
- Complexity 90 101 +11
============================================
Files 177 178 +1
Lines 6620 6639 +19
Branches 783 789 +6
============================================
+ Hits 5243 5257 +14
- Misses 923 926 +3
- Partials 454 456 +2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC93YXRjaGRvZy9Gb3JjZWRNYXhPdXRwdXRSb3dzUnVsZS5zY2FsYQ==) | `64.28% <64.28%> (ø)` | |
| [...in/scala/org/apache/kyuubi/sql/KyuubiSQLConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTUUxDb25mLnNjYWxh) | `100.00% <100.00%> (ø)` | |
| [...rg/apache/kyuubi/sql/KyuubiSparkSQLExtension.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTcGFya1NRTEV4dGVuc2lvbi5zY2FsYQ==) | `92.30% <100.00%> (+0.64%)` | :arrow_up: |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `56.52% <0.00%> (-2.18%)` | :arrow_down: |
| [...pache/kyuubi/sql/KyuubiQueryStagePreparation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlRdWVyeVN0YWdlUHJlcGFyYXRpb24uc2NhbGE=) | `81.37% <0.00%> (+0.98%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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/1085?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 [7da30bd...77aa6ff](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r707880979
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_MAX_OUTPUTROWS) match {
+ case Some(maxOutputRows) => plan match {
Review comment:
> how about match `plan.maxRows > maxOutputRows` ?
if non-limit statement, plan.maxRows is None type，the rule will match non-limit statement and skip statement with limit
the test case illustrate this situation: https://github.com/apache/incubator-kyuubi/pull/1085/files#diff-d26f291114a964b07609603119968579c38f66d8218d1a6de8f092d83397d7b6R1629
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r708266722
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
+ case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)
+ case Origin(Some(_), Some(0)) => GlobalLimit(forcedMaxOutputRows, project)
Review comment:
indeed, this implement is obscure, Shall we have other solution to find tree output root node, I want to know better way to instead this obscure match
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r707298445
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
Review comment:
The rule will insert GlobalLimit node into the SQL tree nodes, it changed original logicalPlan target, it make difference target between Analyzer and Optimizer, yet They should be idempotent
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r708114697
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_MAX_OUTPUTROWS) match {
+ case Some(maxOutputRows) => plan match {
Review comment:
About this issue
1. process `plan.maxRows is none` that node without limit statement
2. `plan.maxRows.get > maxOutputRows` illustrate exists limit statement in query, don't cover user choice, or else user query result exceed expectation as a bug, so skip the situation
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r710190906
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
+ case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)
+ case Origin(Some(_), Some(0)) => GlobalLimit(forcedMaxOutputRows, project)
Review comment:
> Seems a little hack, the `Origin` is used to distinguish if the plan is subquery ?
The `Origin` provides a location for TreeNodes in the context of their origin, it define as below(reference: https://github.com/apache/spark/blob/branch-3.1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala#L49
):
```scala
case class Origin(
line: Option[Int] = None,
startPosition: Option[Int] = None)
The chief output node as root with init position(None or Some(0)), it's not used to distinguish subquery, we can use subquery class to identify
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
+ case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)
+ case Origin(Some(_), Some(0)) => GlobalLimit(forcedMaxOutputRows, project)
Review comment:
> is there any issue if we also add limit for suquery ? I think we don't need care subquery as following:
>
> * scalar subquery; it return at most one row or exception
> * in subquery; it's ok to add limit so we can protect engine's driver (OOM)
> * exist subquery; it will be rewrite to scalar subquery, so it's also safe.
Add limit for subquery may make difference with origin query，my implement will not limit node to subquery
Do you tend to add subquery with above three reasons or not？
--
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 #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#issuecomment-919133338
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 [#1085](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (939d255) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/7da30bd9921dd7957422eab443388335fe809822?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7da30bd) will **increase** coverage by `0.01%`.
> The diff coverage is `72.22%`.
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 #1085 +/- ##
============================================
+ Coverage 79.19% 79.21% +0.01%
- Complexity 90 101 +11
============================================
Files 177 178 +1
Lines 6620 6638 +18
Branches 783 789 +6
============================================
+ Hits 5243 5258 +15
+ Misses 923 922 -1
- Partials 454 458 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC93YXRjaGRvZy9Gb3JjZWRNYXhPdXRwdXRSb3dzUnVsZS5zY2FsYQ==) | `61.53% <61.53%> (ø)` | |
| [...in/scala/org/apache/kyuubi/sql/KyuubiSQLConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTUUxDb25mLnNjYWxh) | `100.00% <100.00%> (ø)` | |
| [...rg/apache/kyuubi/sql/KyuubiSparkSQLExtension.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTcGFya1NRTEV4dGVuc2lvbi5zY2FsYQ==) | `92.30% <100.00%> (+0.64%)` | :arrow_up: |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `56.52% <0.00%> (-2.18%)` | :arrow_down: |
| [.../org/apache/kyuubi/operation/KyuubiOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uLnNjYWxh) | `55.35% <0.00%> (+5.35%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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/1085?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 [7da30bd...939d255](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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] ulysses-you commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r710673547
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
+ case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)
+ case Origin(Some(_), Some(0)) => GlobalLimit(forcedMaxOutputRows, project)
Review comment:
> The Origin provides a location for TreeNodes in the context of their origin
> it's not used to distinguish subquery, we can use subquery class to identify
So I'm still not see why you need match origin here, the origin is used to help track error sql string. Is there any use case that Spark match the origin to do the simialr thing ?
--
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 #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#issuecomment-919133338
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 [#1085](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (13734f8) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/5602c29b3694b3d2fdec6ab8334687180d9135d9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5602c29) will **decrease** coverage by `0.10%`.
> The diff coverage is `60.00%`.
> :exclamation: Current head 13734f8 differs from pull request most recent head c1a88e5. Consider uploading reports for the commit c1a88e5 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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 #1085 +/- ##
============================================
- Coverage 79.24% 79.14% -0.11%
- Complexity 90 97 +7
============================================
Files 177 178 +1
Lines 6620 6640 +20
Branches 783 790 +7
============================================
+ Hits 5246 5255 +9
- Misses 919 926 +7
- Partials 455 459 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC93YXRjaGRvZy9Gb3JjZWRNYXhPdXRwdXRSb3dzUnVsZS5zY2FsYQ==) | `46.66% <46.66%> (ø)` | |
| [...in/scala/org/apache/kyuubi/sql/KyuubiSQLConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTUUxDb25mLnNjYWxh) | `100.00% <100.00%> (ø)` | |
| [...rg/apache/kyuubi/sql/KyuubiSparkSQLExtension.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlTcGFya1NRTEV4dGVuc2lvbi5zY2FsYQ==) | `92.30% <100.00%> (+0.64%)` | :arrow_up: |
| [.../org/apache/kyuubi/operation/KyuubiOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uLnNjYWxh) | `50.00% <0.00%> (-5.36%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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/1085?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 [5602c29...c1a88e5](https://codecov.io/gh/apache/incubator-kyuubi/pull/1085?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] ulysses-you commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r710744382
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add ForcedMaxOutputRows rule for output rows limitation to avoid huge output rows of non_limit query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class ForcedMaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS) match {
+ case Some(forcedMaxOutputRows) => plan match {
+ case insert@InsertIntoDataSourceDirCommand(_, _, Project(_, _), _) =>
+ insert.copy(query = GlobalLimit(forcedMaxOutputRows, insert.query))
+ case project@Project(_, _) => project.origin match {
+ case Origin(None, None) => GlobalLimit(forcedMaxOutputRows, project)
+ case Origin(Some(_), Some(0)) => GlobalLimit(forcedMaxOutputRows, project)
Review comment:
does the project is not root node as following ?
```
plan match {
case _: Project
}
```
can you give a case that we must match origin ?
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r710939964
##########
File path: dev/kyuubi-extension-spark-3-1/src/main/scala/org/apache/kyuubi/sql/watchdog/MaxOutputRowsRule.scala
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.
+ */
+
+package org.apache.kyuubi.sql.watchdog
+
+import org.apache.spark.sql.SparkSession
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{GlobalLimit, LogicalPlan, Project}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.execution.command.InsertIntoDataSourceDirCommand
+
+import org.apache.kyuubi.sql.KyuubiSQLConf
+
+/*
+* Add MaxOutputRows rule for output rows limitation to avoid huge output rows of query unexpectedly
+* mainly applied to cases as below:
+* case 1:
+* {{{
+* INSERT OVERWRITE DIRECTORY (path=STRING)?
+* USING format OPTIONS ([option1_name "option1_value", option2_name "option2_value", ...])
+* SELECT ...
+* }}}
+*
+* case 2:
+* {{{
+* SELECT [c1, c2, ...]
+* }}}
+*
+* case 3:
+* {{{
+* WITH CTE AS (
+* ...)
+* SELECT [c1, c2, ...] FROM CTE ...
+* }}}
+*
+* The Logical Rule add a GlobalLimit node before root project
+* */
+case class MaxOutputRowsRule(session: SparkSession) extends Rule[LogicalPlan] {
+ override def apply(plan: LogicalPlan): LogicalPlan = {
+ if (!plan.resolved) {
+ plan
+ } else {
+ conf.getConf(KyuubiSQLConf.WATCHDOG_MAX_OUTPUTROWS) match {
+ case Some(maxOutputRows) => plan match {
Review comment:
> yes, otherwise this config has no meaning
OK, Final limit according to `spark.sql.watchdog.forcedMaxOutputRows`, commit: https://github.com/apache/incubator-kyuubi/pull/1085/commits/5b0383dd5d46dbb752ecd1bd37fea5f8fbbb60c8
--
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] i7xh commented on a change in pull request #1085: Add forcedMaxOutputRows rule for limitation to avoid huge output unexpectly
Posted by GitBox <gi...@apache.org>.
i7xh commented on a change in pull request #1085:
URL: https://github.com/apache/incubator-kyuubi/pull/1085#discussion_r711451568
##########
File path: dev/kyuubi-extension-spark-3-1/src/test/scala/org/apache/spark/sql/KyuubiExtensionSuite.scala
##########
@@ -1303,4 +1303,96 @@ class KyuubiExtensionSuite extends KyuubiSparkSQLExtensionTest {
}
}
}
+
+ test("test watchdog with query forceMaxOutputRows") {
+
+ withSQLConf(KyuubiSQLConf.WATCHDOG_FORCED_MAXOUTPUTROWS.key -> "10") {
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 1")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(1))
+
+ assert(sql("SELECT * FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2) LIMIT 11")
+ .queryExecution.analyzed.asInstanceOf[GlobalLimit].maxRows.contains(10))
+
+ assert(!sql("SELECT count(*) FROM VALUES(1, 'a'),(2, 'b') AS t(c1, c2)")
+ .queryExecution.analyzed.isInstanceOf[GlobalLimit])
+
+ assert(sql(
+ """
+ |SELECT c1, COUNT(*)
+ |FROM VALUES(1, 'a'), (2, 'b') AS t(c1, c2)
+ |GROUP BY c1
+ |""".stripMargin).queryExecution.analyzed.isInstanceOf[GlobalLimit])
Review comment:
> It's flaky with Spark3.2.0 that the issue of some logical plan has not overridden maxRows is fixed. see [apache/spark#30443](https://github.com/apache/spark/pull/30443).
>
> We can use the test table, see `KyuubiExtensionSuite.setupData`.
_见贤思齐_
I read the [apache/spark#30443](https://github.com/apache/spark/pull/30443). It seems to get more exactly maxRows in Optimizer? I don't understand how affect the test case changed yet, as `VALUES(1, 'a'), (2, 'b') AS t(c1, c2)`?
--
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